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

[Mike Rapoport]

Hi Jinjie,

> 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
that the split you suggest is right.

> Cc: Andrew Morton <akpm at linux-foundation.org>
> Cc: Baoquan He <baoquan.he at linux.dev>
> Cc: Mike Rapoport <rppt at kernel.org>
> Suggested-by: Mike Rapoport <rppt at kernel.org>
> Signed-off-by: Jinjie Ruan <ruanjinjie at 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.

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.



> +
> +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 weak default is never used, all three users override it, and x86,
powerpc and s390 build a helper and a hook that nothing calls.

>  #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.

These are boot protocol constraints and they belong to the architecture.
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
they become generic and RISC-V silently switches from
ELF_CORE_HEADER_ALIGN to 64K in 15/15.


> +
> +	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.

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


> +
> +		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.

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.

> +
> +	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
for adding FDT for example and maybe some other common parts, like, say,
efl headers.

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

-- 
Sincerely yours,
Mike.