Re: [PATCH 2/2] LoongArch: Refactor command line processing

[Youling Tang <youling.tang@linux.dev>]

Hi, Simon

Currently, it is passed through command-line parameters (fdt has not been
used yet), but the readability of the existing command-line parameters is
too poor. By rewriting it to be consistent with the kernel implementation
and using hexadecimal, the readability will be better.

Can Patch2 be applied alone (please ignore Patch1)?

Thanks,
Youling.
On 9/25/25 18:23, Dave Young wrote:
> On Thu, 25 Sept 2025 at 17:52, Youling Tang <youling.tang@linux.dev> wrote:
>> On 9/25/25 17:22, Dave Young wrote:
>>
>> On Thu, 25 Sept 2025 at 14:33, Youling Tang <youling.tang@linux.dev> wrote:
>>
>> From: Youling Tang <tangyouling@kylinos.cn>
>>
>> Refactor the cmdline_add_xxx code flow, and simultaneously display
>> the content of parameters such as initrd in hexadecimal format to
>> improve readability.
>>
>> Signed-off-by: Youling Tang <tangyouling@kylinos.cn>
>> ---
>>   kexec/arch/loongarch/kexec-loongarch.c | 138 ++++++++++---------------
>>   1 file changed, 55 insertions(+), 83 deletions(-)
>>
>> diff --git a/kexec/arch/loongarch/kexec-loongarch.c b/kexec/arch/loongarch/kexec-loongarch.c
>> index 240202f..c2503de 100644
>> --- a/kexec/arch/loongarch/kexec-loongarch.c
>> +++ b/kexec/arch/loongarch/kexec-loongarch.c
>> @@ -35,83 +35,49 @@
>>   #define _O_BINARY 0
>>   #endif
>>
>> -#define CMDLINE_PREFIX "kexec "
>> -static char cmdline[COMMAND_LINE_SIZE] = CMDLINE_PREFIX;
>> +/* Add the "kexec" command line parameter to command line. */
>> +static void cmdline_add_loader(unsigned long *cmdline_tmplen, char *modified_cmdline)
>> +{
>> +       int loader_strlen;
>> +
>> +       loader_strlen = sprintf(modified_cmdline + (*cmdline_tmplen), "kexec ");
>> +       *cmdline_tmplen += loader_strlen;
>> +}
>>
>> Not sure why this is needed,  I guess it is to distinguish the new
>> kernel and original kernel?   As replied in another reply I would
>> suggest adding an extra cmdline in scripts instead of hard coded here,
>>    you need to remove the fake param each time otherwise it will make
>> the cmdline longer and longer after many kexec reboot cycles.
>>
>>
>> In arch_process_options(), the "kexec" parameter will be removed when
>> reusing the current command line.
>>
>> -/* Adds "initrd=start,size" parameters to command line. */
>> -static int cmdline_add_initrd(char *cmdline, unsigned long addr,
>> -               unsigned long size)
>> +/* Add the "initrd=start,size" command line parameter to command line. */
>> +static void cmdline_add_initrd(unsigned long *cmdline_tmplen, char *modified_cmdline,
>> +                              unsigned long initrd_base, unsigned long initrd_size)
>>   {
>> -       int cmdlen, len;
>> -       char str[50], *ptr;
>> -
>> -       ptr = str;
>> -       strcpy(str, " initrd=");
>> -       ptr += strlen(str);
>> -       ultoa(addr, ptr);
>> -       strcat(str, ",");
>> -       ptr = str + strlen(str);
>> -       ultoa(size, ptr);
>> -       len = strlen(str);
>> -       cmdlen = strlen(cmdline) + len;
>> -       if (cmdlen > (COMMAND_LINE_SIZE - 1))
>> -               die("Command line overflow\n");
>> -       strcat(cmdline, str);
>> +       int initrd_strlen;
>>
>> -       return 0;
>> +       initrd_strlen = sprintf(modified_cmdline + (*cmdline_tmplen), "initrd=0x%lx,0x%lx ",
>> +                               initrd_base, initrd_size);
>> +       *cmdline_tmplen += initrd_strlen;
>>   }
>>
>> -/* Adds the appropriate "mem=size@start" options to command line, indicating the
>> - * memory region the new kernel can use to boot into. */
>> -static int cmdline_add_mem(char *cmdline, unsigned long addr,
>> -               unsigned long size)
>> +/*
>> + * Add the "mem=size@start" command line parameter to command line, indicating the
>> + * memory region the new kernel can use to boot into.
>> + */
>> +static void cmdline_add_mem(unsigned long *cmdline_tmplen, char *modified_cmdline,
>> +                           unsigned long mem_start, unsigned long mem_sz)
>>   {
>> -       int cmdlen, len;
>> -       char str[50], *ptr;
>> -
>> -       addr = addr/1024;
>> -       size = size/1024;
>> -       ptr = str;
>> -       strcpy(str, " mem=");
>> -       ptr += strlen(str);
>> -       ultoa(size, ptr);
>> -       strcat(str, "K@");
>> -       ptr = str + strlen(str);
>> -       ultoa(addr, ptr);
>> -       strcat(str, "K");
>> -       len = strlen(str);
>> -       cmdlen = strlen(cmdline) + len;
>> -       if (cmdlen > (COMMAND_LINE_SIZE - 1))
>> -               die("Command line overflow\n");
>> -       strcat(cmdline, str);
>> +       int mem_strlen = 0;
>>
>> -       return 0;
>> +       mem_strlen = sprintf(modified_cmdline + (*cmdline_tmplen), "mem=0x%lx@0x%lx ",
>> +                            mem_sz, mem_start);
>> +       *cmdline_tmplen += mem_strlen;
>>   }
>>
>> Ditto for the mem= param and other similar cases, can this be done out
>> of the kexec-tools c code? it will be more flexible.
>>
>>
>> Currently, we will maintain passing this parameter through the command line, not
>> via FDT like ARM64. In the future, we may consider whether it can be passed through
>> the FDT table in efisystab (but that approach may not be friendly to ELF kernels).
>>
> If the kexec boot depends on the customized mem layout, ideally it
> should be passed with fdt or other method.
> it is reasonable to keep it for the time being.  But the "kexec" extra
> cmdline should not be hard coded in my opinion.
>
> Thanks
> Dave
>