[PATCH] Add method to pass initrd through cmdline

[Simon Horman]

On Tue, May 03, 2022 at 01:50:19PM +0800, Hui Li wrote:
> Hi Simon Horman,
> 
> Thanks for your review.
> 
> On 2022/4/29 下午5:40, Simon Horman wrote:
> > Hi Hui Li,
> > 
> > thanks for your patch.
> > 
> > On Mon, Apr 18, 2022 at 09:36:59AM +0800, Hui Li wrote:
> > > Problem description:
> > > Under loongson platform,use command:
> > > kexec -l vmlinux... --append="root=UUID=28e1..." --initrd=...
> > > kexec -e
> > > quick restart failed.
> > > 
> > > The reason of this problem:
> > > The kernel cannot parse the UUID,UUID is parsed in the initrd.
> > > Loongson platform obtain the initrd through cmdline in kernel.
> > > In kexec-tools, initrd is not added to cmdline.
> > 
> > Is this common to other platforms?
> > If not, is there a reason that Loongson takes this approach?
> > 
> in kernel code,arch/mips/configs/loongson3_defconfig,
> CONFIG_BLK_DEV_INITRD=y is defined.
> In arch/mips/kernel/setup.c file, the following code：
> 
> #ifdef CONFIG_BLK_DEV_INITRD
> 
> 
> static int __init rd_start_early(char *p)
> {
>     unsigned long start = memparse(p, &p);
> 
> #ifdef CONFIG_64BIT
>     /* Guess if the sign extension was forgotten by bootloader */
>     if (start < XKPHYS)
>         start = (int)start;
> #endif
>     initrd_start = start;
>     initrd_end += start;
>     return 0;
> }
> early_param("rd_start", rd_start_early);
> 
> static int __init rd_size_early(char *p)
> {
>     initrd_end += memparse(p, &p);
>     return 0;
> }
> early_param("rd_size", rd_size_early);
> ...
> ...
> ...
> #endif
> 
> 
> 
> The purpose of parsing initrd parameters through cmdline on the loongson is
> to compatible with previous platforms

Thanks. I'm very fine with mechanisms that preserve compatibility.
But if I understand things correctly then this feature is not specific
to Loongson (although it has only been tested on Loongson).

> Maybe other platforms use DTB to parse initrd, but it can be seen that the
> kernel also supports the use of cmdline to parse initrd.
> 
> > > The solution:
> > > Add initrd parameter to cmdline. and add a CONFIG_LOONGSON configuration
> > > to distinguish PAGE_OFFSET between different platforms under mips.
> > > 
> > > Signed-off-by: Hui Li <lihui at loongson.cn>
> > > ---
> > >   configure.ac                     |  5 ++++
> > >   kexec/arch/mips/crashdump-mips.h |  6 ++++-
> > >   kexec/arch/mips/kexec-elf-mips.c | 43 ++++++++++++++++++++++++++++++++
> > >   3 files changed, 53 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/configure.ac b/configure.ac
> > > index cf8e8a2..26bfbcd 100644
> > > --- a/configure.ac
> > > +++ b/configure.ac
> > > @@ -111,6 +111,11 @@ AC_ARG_WITH([booke],
> > >   		AC_DEFINE(CONFIG_BOOKE,1,
> > >   			[Define to build for BookE]))
> > > +AC_ARG_WITH([loongson],
> > > +		AC_HELP_STRING([--with-loongson],[build for loongson]),
> > > +		AC_DEFINE(CONFIG_LOONGSON,1,
> > > +			[Define to build for LoongsoN]))
> > > +
> > >   dnl ---Programs
> > >   dnl To specify a different compiler, just 'export CC=/path/to/compiler'
> > >   if test "${build}" != "${host}" ; then
> > > diff --git a/kexec/arch/mips/crashdump-mips.h b/kexec/arch/mips/crashdump-mips.h
> > > index 7edd859..d53c696 100644
> > > --- a/kexec/arch/mips/crashdump-mips.h
> > > +++ b/kexec/arch/mips/crashdump-mips.h
> > > @@ -5,7 +5,11 @@ struct kexec_info;
> > >   int load_crashdump_segments(struct kexec_info *info, char *mod_cmdline,
> > >   				unsigned long max_addr, unsigned long min_base);
> > >   #ifdef __mips64
> > > -#define PAGE_OFFSET	0xa800000000000000ULL
> > > +#ifdef CONFIG_LOONGSON
> > > +#define PAGE_OFFSET 0xFFFFFFFF80000000ULL
> > > +#else
> > > +#define PAGE_OFFSET 0xa800000000000000ULL
> > > +#endif
> > 
> > Could this be detected at runtime?
> > 
> > It seems awkward to set the PAGE_OFFSET at compile time and
> > thus need separate builds for separate platforms (with no warning
> > if the wrong one is used).
> > 
> Thank you very much for this good suggestion.
> I see that there is a way to get different platform information at runtime
> via "/proc/cpuinfo", which is used to distinguish PAGE_OFFSET for different
> platforms.

Looking at ARM and arm64 implementations of get_kernel_page_offset()
they seem to rely on /proc/kallsyms for autodetection of page_offset.

Perhaps something similar could be done for MIPS.

Also, as an asside, it seems at a glance that ARM and adm64 are the same in
this regard. So there seems to be some scope for consolidation.

> 
> > >   #define MAXMEM		0
> > >   #else
> > >   #define PAGE_OFFSET	0x80000000
> > > diff --git a/kexec/arch/mips/kexec-elf-mips.c b/kexec/arch/mips/kexec-elf-mips.c
> > > index a2d11fc..1de418e 100644
> > > --- a/kexec/arch/mips/kexec-elf-mips.c
> > > +++ b/kexec/arch/mips/kexec-elf-mips.c
> > > @@ -40,6 +40,44 @@ static const int probe_debug = 0;
> > >   #define CMDLINE_PREFIX "kexec "
> > >   static char cmdline_buf[COMMAND_LINE_SIZE] = CMDLINE_PREFIX;
> > > +/* Converts unsigned long to ascii string. */
> > > +static void ultoa(unsigned long i, char *str)
> > > +{
> > > +	int j = 0, k;
> > > +	char tmp;
> > > +
> > > +	do {
> > > +		str[j++] = i % 10 + '0';
> > > +	} while ((i /= 10) > 0);
> > > +	str[j] = '\0';
> > 
> > Could the above be achieved using printf?
> > 
> > > +	/* Reverse the string. */
> > > +	for (j = 0, k = strlen(str) - 1; j < k; j++, k--) {
> > > +		tmp = str[k];
> > > +		str[k] = str[j];
> > > +		str[j] = tmp;
> > > +	}
> > 
> > I'm confused as to why the string needs to be reversed.
> > Could it be avoided by byte-swapping 'i' before rendering it to a string?
> > Thanks a lot for your suggestions on these details.
> ultoa() function is used in many places.I see many other platforms have the
> same implementation like:
> /kexec/arch/i386/crashdump-x86.c
> /kexec/arch/ppc64/crashdump-ppc64.c
> /kexec/arch/ppc64/crashdump-sh.c
> 
> So, Is it possible to keep the existing implementation ?

Ok, sure.

> > > +}
> > > +
> > > +/* Adds initrd parameters to command line. */
> > > +static int cmdline_add_initrd(char *cmdline, unsigned long addr, char *new_para)
> > > +{
> > > +	int cmdlen, len;
> > > +	char str[30], *ptr;
> > > +
> > > +	ptr = str;
> > > +	strcpy(str, new_para);
> > > +	ptr += strlen(str);
> > > +	ultoa(addr, ptr);
> > > +	len = strlen(str);
> > > +	cmdlen = strlen(cmdline) + len;
> > > +	if (cmdlen > (COMMAND_LINE_SIZE - 1))
> > > +		die("Command line overflow\n");
> > > +	strcat(cmdline, str);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +
> > >   int elf_mips_probe(const char *buf, off_t len)
> > >   {
> > >   	struct mem_ehdr ehdr;
> > > @@ -171,6 +209,11 @@ int elf_mips_load(int argc, char **argv, const char *buf, off_t len,
> > >   		/* Now that the buffer for initrd is prepared, update the dtb
> > >   		 * with an appropriate location */
> > >   		dtb_set_initrd(&dtb_buf, &dtb_length, initrd_base, initrd_base + initrd_size);
> > > +
> > > +		/* Add the initrd parameters to cmdline */
> > > +		cmdline_add_initrd(cmdline_buf, PAGE_OFFSET + initrd_base, " rd_start=");
> > > +		cmdline_add_initrd(cmdline_buf, initrd_size, " rd_size=");
> > > +
> > 
> > Will this be safe for existing use-cases?
> > 
> 
> This modification is safe for other existing use-cases.
> Now this method is only for the loongson platform to pass the initrd
> correctly.  At this stage, this change is no problem to use on the loongson
> platform, and it will not affect other platforms.
> This part can be further constrained, only for the loongson platform.
> 
> let me think about the above and try to
> find a proper way, and then I will send a v3 patch.

I think it would be best to avoid compile-time constraints.
But if a runtime constraint can ensure this runs only for tested
platforms and existing behaviour is preserved for other platforms (if any)
then I think we would be in good shape.

Kind regards,
Simon