Message-ID: <aJwFa8x5BQMouB1y@pie>
Date: Wed, 13 Aug 2025 03:24:27 +0000
From: Yao Zi <ziyao@...root.org>
To: Youling Tang <youling.tang@...ux.dev>,
	Huacai Chen <chenhuacai@...nel.org>
Cc: WANG Xuerui <kernel@...0n.name>, Baoquan He <bhe@...hat.com>,
	kexec@...ts.infradead.org, loongarch@...ts.linux.dev,
	linux-kernel@...r.kernel.org, Youling Tang <tangyouling@...inos.cn>
Subject: Re: [PATCH 2/6] LoongArch: Add kexec_file support

On Wed, Aug 13, 2025 at 10:18:12AM +0800, Youling Tang wrote:
> Hi, Yao
> On 2025/8/12 17:43, Yao Zi wrote:
> > On Tue, Aug 12, 2025 at 03:06:23PM +0800, Youling Tang wrote:
> > > On 2025/8/12 14:15, Youling Tang wrote:
> > > > Hi, Yao
> > > > On 2025/8/12 01:06, Yao Zi wrote:
> > > > > On Mon, Aug 11, 2025 at 05:26:55PM +0800, Youling Tang wrote:
> > > > > > From: Youling Tang <tangyouling@...inos.cn>
> > > > > > 
> > > > > > This patch adds support for kexec_file on LoongArch.
> > > > > > 
> > > > > > The image_load() as two parts:
> > > > > > - the first part loads the kernel image (vmlinuz.efi or vmlinux.efi)
> > > > > > - the second part loads other segments (eg: initrd, cmdline)
> > > > > > 
> > > > > > Currently, pez(vmlinuz.efi) and pei(vmlinux.efi) format images
> > > > > > are supported,
> > > > > > but ELF format is not supported.
> > > > > > 
> > > > > > Signed-off-by: Youling Tang <tangyouling@...inos.cn>
> > > > > > ---
> > > > > >    arch/loongarch/Kconfig                     |   8 ++
> > > > > >    arch/loongarch/include/asm/image.h         |  18 ++++
> > > > > >    arch/loongarch/include/asm/kexec.h         |  12 +++
> > > > > >    arch/loongarch/kernel/Makefile             |   1 +
> > > > > >    arch/loongarch/kernel/kexec_image.c        | 112
> > > > > > +++++++++++++++++++++
> > > > > >    arch/loongarch/kernel/machine_kexec.c      |  33 ++++--
> > > > > >    arch/loongarch/kernel/machine_kexec_file.c |  46 +++++++++
> > > > > >    7 files changed, 219 insertions(+), 11 deletions(-)
> > > > > >    create mode 100644 arch/loongarch/kernel/kexec_image.c
> > > > > >    create mode 100644 arch/loongarch/kernel/machine_kexec_file.c
> > ...
> > 
> > > > > > diff --git a/arch/loongarch/kernel/kexec_image.c
> > > > > > b/arch/loongarch/kernel/kexec_image.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..fdd1845b4e2e
> > > > > > --- /dev/null
> > > > > > +++ b/arch/loongarch/kernel/kexec_image.c
> > ...
> > 
> > > > > > +    /*
> > > > > > +     * The location of the kernel segment may make it
> > > > > > impossible to satisfy
> > > > > > +     * the other segment requirements, so we try repeatedly to find a
> > > > > > +     * location that will work.
> > > > > > +     */
> > > > > > +    while ((ret = kexec_add_buffer(&kbuf)) == 0) {
> > > > > > +        /* Try to load additional data */
> > > > > > +        kernel_segment = &image->segment[kernel_segment_number];
> > > > > > +        ret = load_other_segments(image, kernel_segment->mem,
> > > > > > +                      kernel_segment->memsz, initrd,
> > > > > > +                      initrd_len, cmdline, cmdline_len);
> > > > > > +        if (!ret)
> > > > > > +            break;
> > > > > > +
> > > > > > +        /*
> > > > > > +         * We couldn't find space for the other segments; erase the
> > > > > > +         * kernel segment and try the next available hole.
> > > > > > +         */
> > > > > > +        image->nr_segments -= 1;
> > > > > > +        kbuf.buf_min = kernel_segment->mem + kernel_segment->memsz;
> > > > > > +        kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
> > > > > > +    }
> > > > > > +
> > > > > > +    if (ret) {
> > > > > > +        pr_err("Could not find any suitable kernel location!");
> > > > > > +        return ERR_PTR(ret);
> > > > > > +    }
> > > > > > +
> > > > > > +    kernel_segment = &image->segment[kernel_segment_number];
> > > > > > +
> > > > > > +    /* Make sure the second kernel jumps to the correct
> > > > > > "kernel_entry". */
> > > > > > +    image->start = kernel_segment->mem + h->kernel_entry -
> > > > > > text_offset;
> > > > > A non-relocatable loongarch kernel cannot be loaded to arbitrary
> > > > > address. Thus this loading function seems to only work for relocatable
> > > > > kernels, maybe it's better to leave a comment indicating the limitation.
> > > > > 
> > > > > For now, we don't seem to have a way to find out whether the kernel is
> > > > > relocatable (for example, a flag in kernel image header), so it's
> > > > > impossible to point out whether the loaded kernel boots fine with
> > > > > arbitrary loading address...
> > > > LoongArch enables the relocation of the kernel when the kdump
> > > > feature is enabled.
> > > > 
> > > > config ARCH_SELECTS_CRASH_DUMP
> > > >          def_bool y
> > > >          depends on CRASH_DUMP
> > > >          select RELOCATABLE
> > > > 
> > This only means the currently-running kernel is relocatable, not the one
> > being exec'ed, right?
> No.
> > > When enabling KEXEC_FILE, the RELOCATABLE configuration should
> > > also be enabled. Both kexec and kdump require this.
> > I'm not sure whether you're talking about the running kernel or the one
> > that is going to be exec'ed. This method of kernel loading requires the
> > exec'ed kernel being relocatable, not the currently running one.
> > 
> > And I think it's totally reasonable to use KEXEC_FILE for non-crash-dump
> > purpose, for example, linuxboot. It'll be confusing to the user if the
> > system just hangs after booting a non-relocatable kernel, which is hard
> > to debug.
> > 
> > Thus IMHO we should ideally refuse to load non-relocatable kernels, or
> > add a FIXME comment to indicate the situation that it's impossible to
> > determine whether the exec'ed image is relocatable.
> The first kernel and the second kernel are generally the same kernel
> (the same image).

This isn't true. There're real-world cases using kexec-file to load and
boot an unrelated kernel image. Please refer to petitboot[1] and
linuxboot[2] which uses kexec to act as a bootloader.

> When KEXEC_FILE is enabled and RELOCATEABLE
> is enabled by default, it has been forcibly guaranteed that both the
> first kernel and the second kernel are relocatable kernels regardless
> of kexec/kdump operations.

This cannot be guaranteed since the first and second kernel could be
completely unrelated. Please see my previous comment.

> Unless the second kernel it loads is an older version of the kernel (the
> older version of the kernel does not use the default configuration, with
> CONFIG_KEXEC enabled but CONFIG_CRASH_DUMP disabled,

> this is not acorrect usage).

This may be incorrect for kdump's use case, but kexec-file isn't only
meant to be used with kdump: it just loads an arbitrary kernel image and
executes it, no matter whether it's for capturing a dump, booting a
newer or older kernel, or etc, and there's no guarantee about features
enabled in the second kernel. It's incorrect to assume the user only
loads relocatable images with kexec-file.

> Thanks,
> Youling.
> > > Youling.
> > > > After enabling the relocation, LoongArch is the PIE kernel. For
> > > > more details, please refer to commit d8da19fbdedd ("LoongArch:
> > > > Add support for kernel relocation")
> > Best regards.
> > Yao Zi
> 

Thanks,
Yao Zi

[1]: https://github.com/open-power/petitboot
[2]: https://github.com/linuxboot/linuxboot

Powered by blists - more mailing lists