Message-ID: <CAAhV-H4p4_HncS5jc29c1yEFG-zqmxZ_DLXwTxjsT-t7kh8WRQ@mail.gmail.com>
Date: Thu, 10 Jul 2025 12:28:14 +0800
From: Huacai Chen <chenhuacai@...nel.org>
To: Ming Wang <wangming01@...ngson.cn>
Cc: Yanteng Si <si.yanteng@...ux.dev>, 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, 
	lixuefeng@...ngson.cn, chenhuacai@...ngson.cn, gaojuxin@...ngson.cn
Subject: Re: [PATCH] LoongArch: Support mem=SIZE kernel parameter

On Mon, Jul 7, 2025 at 6:03 PM Ming Wang <wangming01@...ngson.cn> wrote:
>
> Hi Yanteng, Huacai,
>
> 在 2025/7/7 10:33, Yanteng Si 写道:
> >
> > 在 7/3/25 9:36 AM, Ming Wang 写道:
> >> 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 have an idea: what if we move the destructive code into the if block?
>
> @Yanteng,
> That's an excellent suggestion. You are right. Moving the destructive
> memblock_remove() logic inside the if (*p == '@') block is indeed a much
> cleaner way to structure the code. It improves readability by making the
> logic for each case self-contained within a direct if/else structure.
>
> @Huacai,
> Yanteng proposed a great improvement to the patch structure.
>
> ```
> static int __init early_parse_mem(char *p)
> {
>      // ...
>      size = memparse(p, &p);
>      if (*p == '@') {
>          // Handle 'mem=SIZE@...RT'
>          // The destructive memblock_remove() goes here.
>          // ...
>          // memblock_add_node()
>      } else {
>          // Handle 'mem=SIZE'
>          // The non-destructive memblock_enforce_memory_limit() goes here.
>      }
>      return 0;
> }
> ```
>
> Before I send out a v2, I'd like to ask for your opinion on this
> proposed change as well. Do you agree that this revised structure is the
> better approach?
Nested if conditions make the logic unclear, so I prefer the current patch.

Huacai

>
> Best regards,
> Ming
>
> >
> >
> > Thanks,
> >
> > Yanteng
> >
> >>
> >> 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