Re: [PATCH v7 12/15] kexec: Add kexec_load_other_segments() helper

[Jinjie Ruan <ruanjinjie@huawei.com>]

在 2026/11/10 18:04, Mike Rapoport 写道:
> Hi Jinjie,

Hi Mike,

> 
>> arm64, LoongArch and RISC-V duplicate the elf core header and initrd
>> loading.  Move it into a common helper with one arch hook for the boot
>> parameters; a failure releases everything added for the attempt.
> 
> I like the idea of pulling out the common code, but I'm not convinced

Thank you for the review.

> that the split you suggest is right.

Yes, the current solution is straightforward.

> 
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Baoquan He <baoquan.he@linux.dev>
>> Cc: Mike Rapoport <rppt@kernel.org>
>> Suggested-by: Mike Rapoport <rppt@kernel.org>
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> ---
>>  include/linux/kexec.h | 15 ++++++++
>>  kernel/kexec_file.c   | 90 +++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 105 insertions(+)
>>
>> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
>> index faba26f871a4a..de1d83ddc38bd 100644
>> --- a/include/linux/kexec.h
>> +++ b/include/linux/kexec.h
>> @@ -259,6 +259,21 @@ int kexec_kernel_verify_pe_sig(const char *kernel, unsigned long kernel_len);
>>  extern int kexec_add_buffer(struct kexec_buf *kbuf);
>>  int kexec_locate_mem_hole(struct kexec_buf *kbuf);
>>  
>> +struct kexec_load_params {
>> +	unsigned long kernel_load_addr;
>> +	unsigned long kernel_size;
>> +	char *initrd;
>> +	unsigned long initrd_len;
>> +	char *cmdline;
>> +	unsigned long cmdline_len;
>> +};
> 
> initrd and cmdline are already in image->initrd_buf, initrd_buf_len,
> cmdline_buf and cmdline_buf_len, that's where fops->load() gets them from.

OK, will delete them.

> 
> And the outputs are scattered: elf_load_addr goes to the image, the initrd
> address is passed as a separate argument to the hook, and LoongArch infers
> "there is an initrd" from initrd_load_addr != 0.

These are currently just being shoehorned in.

> 
> 
> 
>> +
>> +int arch_kexec_setup_other_segments(struct kimage *image,
>> +				    struct kexec_load_params *params,
>> +				    unsigned long initrd_load_addr);
>> +int kexec_load_other_segments(struct kimage *image,
>> +			      struct kexec_load_params *params, bool top_down);
>> +
> 
> The call chain is now fops->load() in arch -> kexec_load_other_segments()
> in core -> arch_kexec_setup_other_segments() back in arch. That's a
> midlayer: the core helper owns the sequence and the arch fills a hole in
> the middle of it.

The new refactoring will follow this principle to ensure that the
framework code is in control of the order.

> 
> The weak default is never used, all three users override it, and x86,
> powerpc and s390 build a helper and a hook that nothing calls.

Yes, x86 and powerpc has not used it, Some adjustments need for the rework.

> 
>>  #ifndef arch_kexec_locate_mem_hole
>>  /**
>>   * arch_kexec_locate_mem_hole - Find free memory to place the segments.
>> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
>> index 62d4cba278a57..8b80f1186875b 100644
>> --- a/kernel/kexec_file.c
>> +++ b/kernel/kexec_file.c
>> @@ -1295,3 +1295,93 @@ int kexec_purgatory_get_set_symbol(struct kimage *image, const char *name,
>>  	return 0;
>>  }
>>  #endif /* CONFIG_ARCH_SUPPORTS_KEXEC_PURGATORY */
>> +
>> +int __weak arch_kexec_setup_other_segments(struct kimage *image,
>> +					   struct kexec_load_params *params,
>> +					   unsigned long initrd_load_addr)
>> +{
>> +	return 0;
>> +}
>> +
>> +int kexec_load_other_segments(struct kimage *image,
>> +			      struct kexec_load_params *params, bool top_down)
>> +{
>> +	struct kexec_buf kbuf = {
>> +		.image = image,
>> +		/* Do not allocate anything below the kernel */
>> +		.buf_min = params->kernel_load_addr + params->kernel_size,
>> +		.buf_max = top_down ? PFN_PHYS(max_low_pfn) : ULONG_MAX,
>> +	};
> 
> top_down does not describe a direction, it selects "RISC-V policy" vs
> "arm64 policy": the upper limit for all buffers, the initrd direction and
> the 32G initrd window.

Yes its the topdown of initrd and distinguish between different
architectures is a bit of a stretch here.

Right! arm64 has its unique boot protocol restrictions. Here, RISC-V has
a bug fix that requires specifying a memory top limit.

> 
> These are boot protocol constraints and they belong to the architecture.

Agree

> The 32G window comes from arm64 booting.rst and the SZ_64K alignment below
> is "largest supported page size" on arm64 and LoongArch. With this patch

Yes.

> they become generic and RISC-V silently switches from
> ELF_CORE_HEADER_ALIGN to 64K in 15/15.

There are indeed some function changes, which are also mentioned
in the commit message.

> 
> 
>> +
>> +	unsigned long orig_segments = image->nr_segments;
>> +	unsigned long initrd_load_addr = 0;
>> +	int ret = 0;
>> +
>> +#ifdef CONFIG_CRASH_DUMP
>> +	if (image->type == KEXEC_TYPE_CRASH) {
>> +		ret = crash_prepare_headers(true, &kbuf.buffer, &kbuf.bufsz, NULL);
>> +		if (ret) {
>> +			pr_err("Preparing elf core header failed\n");
>> +			goto out_err;
>> +		}
>> +
>> +		if (unlikely(image->elf_headers))
>> +			vfree(image->elf_headers);
> 
> The headers don't depend on where the kernel is placed, yet they are
> rebuilt on every iteration of the arm64 and LoongArch retry loops. That's
> why the vfree() above is needed, and with the out_err path freeing them
> it's now dead code.

Yes, it need deleted now.

> 
> Building the headers once and only placing them in the loop removes the
> leak instead of fixing it.

I agree, I had a similar fix attempt before, where I only allocated once
to avoid placing it in a loop.

> 
> 
>> +
>> +		image->elf_headers = kbuf.buffer;
>> +		image->elf_headers_sz = kbuf.bufsz;
>> +
>> +		kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>> +		kbuf.memsz = kbuf.bufsz;
>> +		kbuf.buf_align = SZ_64K; /* largest supported page size */
>> +		kbuf.top_down = true;
>> +
>> +		ret = kexec_add_buffer(&kbuf);
>> +		if (ret)
>> +			goto out_err;
>> +
>> +		image->elf_load_addr = kbuf.mem;
>> +
>> +		kexec_dprintk("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
>> +			      image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
>> +	}
>> +#endif
>> +
>> +	if (params->initrd) {
>> +		kbuf.buffer = params->initrd;
>> +		kbuf.bufsz = params->initrd_len;
>> +		kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>> +		kbuf.memsz = params->initrd_len;
>> +		kbuf.buf_align = 0;
>> +		/* within 1GB-aligned window of up to 32GB in size */
>> +		if (!top_down)
>> +			kbuf.buf_max = round_down(params->kernel_load_addr, SZ_1G) +
>> +					(unsigned long)SZ_1G * 32;
>> +		kbuf.top_down = top_down;
>> +
>> +		ret = kexec_add_buffer(&kbuf);
>> +		if (ret)
>> +			goto out_err;
>> +		initrd_load_addr = kbuf.mem;
>> +
>> +		kexec_dprintk("Loaded initrd at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
>> +			      initrd_load_addr, kbuf.bufsz, kbuf.memsz);
>> +	}
>> +
>> +	ret = arch_kexec_setup_other_segments(image, params,
>> +					      initrd_load_addr);
>> +	if (ret)
>> +		goto out_err;
>> +
>> +	return 0;
>> +
>> +out_err:
>> +	while (image->nr_segments > orig_segments) {
>> +		kexec_free_segment_cma(image, image->nr_segments - 1);
>> +		image->nr_segments--;
>> +	}
>> +
>> +	kexec_free_elf_headers(image);
> 
> This drops segments added by the arch hook (DTB, RISC-V purgatory, arm64
> dm-crypt keys) without knowing anything about the state attached to them,
> like image->dm_crypt_keys_addr or the purgatory info. The hook, in turn,
> frees its buffers but not its segments.

Yes

> 
> Nobody owns the unwind, so "a failure releases everything added for the
> attempt" is not quite what happens.
> 
> The conversions also bury ordering changes in "simplify" patches: on arm64
> dm-crypt keys move after the initrd, and on RISC-V the initrd moves before
> the purgatory. And dm-crypt keys loading is not an arm64 boot parameter,
> x86 and powerpc use it as well.
>

Yes, the segment allocation order is indeed changed.


>> +
>> +	return ret;
>> +}
> 
> 
> What I had in mind was to make the core provide building blocks and let
> the architectures compose them, e.g. a common rollback function that
> frees CMAs and drops segments above nr. The retry loops in arm64 and
> LoongArch image loaders can use it instead of open coding
> kexec_free_segment_cma() and nr_segments--. There also could be a helper

I agree.

> for adding FDT for example and maybe some other common parts, like, say,
> efl headers.


I am trying out your refactoring ideas so that x86 and PowerPC can also
use these newly added small hook functions.

> 
> Arch's load_other_segments() could use these helpers and still define
> their own policy for segment placement.

This will eliminate the issue of changes in the above sequence or the
problem of the startup protocol being modified.

Thank you again for your review and suggestions for refactoring. I will
thoroughly consider the issues you raised, revise the code, and conduct
more testing.

> 

-- 
Best regards,
Jinjie