LinuxLists.cc - [PATCH 0/5] kexec_file

Attachments:

signature.asc (497.00 B)

2020-06-01 14:59:58

by Russell King (Oracle)

[permalink] [raw]

Subject: Re: [PATCH 3/5] arm: decompressor: define a new zImage tag

On Mon, Jun 01, 2020 at 04:27:52PM +0200, Łukasz Stelmach wrote:
> Add DCSZ tag which holds dynamic memory (stack, bss, malloc pool)
> requirements of the decompressor code.

Why do we need to know the stack and BSS size, when the userspace
kexec tool doesn't need to know this to perform the same function.

>
> Signed-off-by: Łukasz Stelmach <[email protected]>
> ---
> arch/arm/boot/compressed/vmlinux.lds.S | 9 ++++++++-
> arch/arm/include/asm/image.h | 13 +++++++++++++
> 2 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
> index 308e9cd6a897..dcfdb3209c90 100644
> --- a/arch/arm/boot/compressed/vmlinux.lds.S
> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
> @@ -39,6 +39,11 @@ SECTIONS
> LONG(ARM_ZIMAGE_MAGIC3)
> LONG(ZIMAGE_MAGIC(__piggy_size_addr - _start))
> LONG(ZIMAGE_MAGIC(_kernel_bss_size))
> + LONG(ZIMAGE_MAGIC(5))
> + LONG(ARM_ZIMAGE_MAGIC4)
> + LONG(ZIMAGE_MAGIC(_end - __bss_start))
> + LONG(ZIMAGE_MAGIC(_stack_end - _stack_start))
> + LONG(ZIMAGE_MAGIC(_malloc_size))
> LONG(0)
> _table_end = .;
> }
> @@ -108,10 +113,12 @@ SECTIONS
> . = BSS_START;
> __bss_start = .;
> .bss : { *(.bss) }
> + . = ALIGN(8); /* the stack must be 64-bit aligned and adjoin bss */
> _end = .;
>
> - . = ALIGN(8); /* the stack must be 64-bit aligned */
> + _stack_start = .;
> .stack : { *(.stack) }
> + _stack_end = .;
>
> PROVIDE(__pecoff_data_size = ALIGN(512) - ADDR(.data));
> PROVIDE(__pecoff_end = ALIGN(512));
> diff --git a/arch/arm/include/asm/image.h b/arch/arm/include/asm/image.h
> index d5c18a0f6a34..624438740f23 100644
> --- a/arch/arm/include/asm/image.h
> +++ b/arch/arm/include/asm/image.h
> @@ -15,6 +15,7 @@
> #define ARM_ZIMAGE_MAGIC1 ZIMAGE_MAGIC(0x016f2818)
> #define ARM_ZIMAGE_MAGIC2 (0x45454545)
> #define ARM_ZIMAGE_MAGIC3 ZIMAGE_MAGIC(0x5a534c4b)
> +#define ARM_ZIMAGE_MAGIC4 ZIMAGE_MAGIC(0x5a534344)
>
> #ifndef __ASSEMBLY__
>
> @@ -43,6 +44,18 @@ struct arm_zimage_tag {
> } u;
> };
>
> +struct arm_zimage_tag_dc {
> + struct tag_header hdr;
> + union {
> +#define ZIMAGE_TAG_DECOMP_SIZE ARM_ZIMAGE_MAGIC4
> + struct zimage_decomp_size {
> + __le32 bss_size;
> + __le32 stack_size;
> + __le32 malloc_size;
> + } decomp_size;
> + } u;
> +};
> +
> #endif /* __ASSEMBLY__ */
>
> #endif /* __ASM_IMAGE_H */
> --
> 2.26.2
>
>

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

2020-06-01 15:13:11

by Russell King (Oracle)

[permalink] [raw]

Subject: Re: [PATCH 5/5] arm: kexec_file: load zImage or uImage, initrd and dtb

On Mon, Jun 01, 2020 at 04:27:54PM +0200, Łukasz Stelmach wrote:
> This is kexec_file_load implementation for ARM. It loads zImage and
> initrd from file descripters and resuses DTB.
>
> Most code is derived from arm64 kexec_file_load implementation
> and from kexec-tools.

Please make the uImage loader able to be configured out of the
kernel; it's a legacy image format now, and some of us just don't
use it anymore. That way, those who wish to have it can, and
those who wish not to can have a smaller kernel.

> Cc: AKASHI Takahiro <[email protected]>
> Signed-off-by: Łukasz Stelmach <[email protected]>
> ---
> arch/arm/Kconfig | 15 ++
> arch/arm/include/asm/image.h | 26 ++++
> arch/arm/include/asm/kexec.h | 14 ++
> arch/arm/kernel/Makefile | 5 +-
> arch/arm/kernel/kexec_uimage.c | 80 ++++++++++
> arch/arm/kernel/kexec_zimage.c | 199 +++++++++++++++++++++++++
> arch/arm/kernel/machine_kexec.c | 11 +-
> arch/arm/kernel/machine_kexec_file.c | 209 +++++++++++++++++++++++++++
> 8 files changed, 554 insertions(+), 5 deletions(-)
> create mode 100644 arch/arm/kernel/kexec_uimage.c
> create mode 100644 arch/arm/kernel/kexec_zimage.c
> create mode 100644 arch/arm/kernel/machine_kexec_file.c
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index c77c93c485a0..6adb849cb304 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1917,6 +1917,21 @@ config KEXEC
> is properly shutdown, so do not be surprised if this code does not
> initially work for you.
>
> +config KEXEC_FILE
> + bool "Kexec file based system call (EXPERIMENTAL)"
> + depends on (!SMP || PM_SLEEP_SMP)
> + depends on USE_OF
> + select KEXEC_CORE
> + select CRC32
> + help
> + This is new version of kexec system call. This system call is
> + file based and takes file descriptors as system call argument
> + for kernel and initramfs as opposed to list of segments as
> + accepted by previous system call.
> +
> + The kernel to be loaded MUST support Flattened Device Tree
> + (selected with CONFIG_USE_OF).
> +
> config ATAGS_PROC
> bool "Export atags in procfs"
> depends on ATAGS && KEXEC
> diff --git a/arch/arm/include/asm/image.h b/arch/arm/include/asm/image.h
> index 624438740f23..95f23837b04f 100644
> --- a/arch/arm/include/asm/image.h
> +++ b/arch/arm/include/asm/image.h
> @@ -8,8 +8,13 @@
> (((x) >> 8) & 0x0000ff00) | \
> (((x) << 8) & 0x00ff0000) | \
> (((x) << 24) & 0xff000000))
> +#define UIMAGE_MAGIC(x) (x)
> #else
> #define ZIMAGE_MAGIC(x) (x)
> +#define UIMAGE_MAGIC(x) ((((x) >> 24) & 0x000000ff) | \
> + (((x) >> 8) & 0x0000ff00) | \
> + (((x) << 8) & 0x00ff0000) | \
> + (((x) << 24) & 0xff000000))
> #endif
>
> #define ARM_ZIMAGE_MAGIC1 ZIMAGE_MAGIC(0x016f2818)
> @@ -17,6 +22,12 @@
> #define ARM_ZIMAGE_MAGIC3 ZIMAGE_MAGIC(0x5a534c4b)
> #define ARM_ZIMAGE_MAGIC4 ZIMAGE_MAGIC(0x5a534344)
>
> +#define ARM_UIMAGE_MAGIC UIMAGE_MAGIC(0x27051956)
> +#define ARM_UIMAGE_NAME_LEN 32
> +#define ARM_UIMAGE_TYPE_KERNEL 2
> +#define ARM_UIMAGE_TYPE_KERNEL_NOLOAD 14
> +#define ARM_UIMAGE_ARCH_ARM 2
> +
> #ifndef __ASSEMBLY__
>
> #include <linux/types.h>
> @@ -33,6 +44,21 @@ struct arm_zimage_header {
> __le32 extension_tag_offset;
> };
>
> +struct arm_uimage_header {
> + __be32 magic;
> + __be32 hdr_crc;
> + __be32 time;
> + __be32 size;
> + __be32 load;
> + __be32 entry;
> + __be32 crc;
> + __u8 os;
> + __u8 arch;
> + __u8 type;
> + __u8 comp;
> + __u8 name[ARM_UIMAGE_NAME_LEN];
> +};
> +
> struct arm_zimage_tag {
> struct tag_header hdr;
> union {
> diff --git a/arch/arm/include/asm/kexec.h b/arch/arm/include/asm/kexec.h
> index 22751b5b5735..fda35afa7195 100644
> --- a/arch/arm/include/asm/kexec.h
> +++ b/arch/arm/include/asm/kexec.h
> @@ -83,6 +83,20 @@ static inline struct page *boot_pfn_to_page(unsigned long boot_pfn)
> }
> #define boot_pfn_to_page boot_pfn_to_page
>
> +#ifdef CONFIG_KEXEC_FILE
> +
> +extern const struct kexec_file_ops kexec_zimage_ops;
> +extern const struct kexec_file_ops kexec_uimage_ops;
> +
> +struct kimage;
> +
> +extern int load_other_segments(struct kimage *image,
> + unsigned long kernel_load_addr, unsigned long kernel_size,
> + char *initrd, unsigned long initrd_len,
> + unsigned long initrd_offset, char *cmdline);
> +
> +#endif /* CONFIG_KEXEC_FILE */
> +
> #endif /* __ASSEMBLY__ */
>
> #endif /* CONFIG_KEXEC */
> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> index 89e5d864e923..466c683bb551 100644
> --- a/arch/arm/kernel/Makefile
> +++ b/arch/arm/kernel/Makefile
> @@ -3,6 +3,7 @@
> # Makefile for the linux kernel.
> #
>
> +CFLAGS_kexec_zimage.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
> CPPFLAGS_vmlinux.lds := -DTEXT_OFFSET=$(TEXT_OFFSET)
> AFLAGS_head.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
>
> @@ -56,7 +57,9 @@ obj-$(CONFIG_FUNCTION_TRACER) += entry-ftrace.o
> obj-$(CONFIG_DYNAMIC_FTRACE) += ftrace.o insn.o patch.o
> obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o insn.o patch.o
> obj-$(CONFIG_JUMP_LABEL) += jump_label.o insn.o patch.o
> -obj-$(CONFIG_KEXEC) += machine_kexec.o relocate_kernel.o
> +obj-$(CONFIG_KEXEC_CORE) += machine_kexec.o relocate_kernel.o
> +obj-$(CONFIG_KEXEC_FILE) += machine_kexec_file.o kexec_zimage.o \
> + kexec_uimage.o
> # Main staffs in KPROBES are in arch/arm/probes/ .
> obj-$(CONFIG_KPROBES) += patch.o insn.o
> obj-$(CONFIG_OABI_COMPAT) += sys_oabi-compat.o
> diff --git a/arch/arm/kernel/kexec_uimage.c b/arch/arm/kernel/kexec_uimage.c
> new file mode 100644
> index 000000000000..47033574e24e
> --- /dev/null
> +++ b/arch/arm/kernel/kexec_uimage.c
> @@ -0,0 +1,80 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Kexec uImage loader
> + *
> + * Copyright (C) 2020 Samsung Electronics
> + * Author: Łukasz Stelmach <[email protected]>
> + */
> +
> +#define pr_fmt(fmt) "kexec_file(uImage): " fmt
> +
> +#include <asm/image.h>
> +#include <linux/crc32.h>
> +#include <linux/err.h>
> +#include <linux/kexec.h>
> +
> +#define crc32_ones(crc, buf, len) \
> + (crc32(crc ^ 0xffffffff, buf, len) ^ 0xffffffff)
> +
> +static int uimage_probe(const char *uimage_buf, unsigned long uimage_len)
> +{
> + const struct arm_uimage_header *h =
> + (struct arm_uimage_header *) uimage_buf;
> + struct arm_uimage_header uhdr;
> + unsigned long zoff = sizeof(struct arm_uimage_header);
> + uint32_t crc;
> +
> + if (h->magic != ARM_UIMAGE_MAGIC)
> + return -EINVAL;
> +
> + if (h->type != ARM_UIMAGE_TYPE_KERNEL &&
> + h->type != ARM_UIMAGE_TYPE_KERNEL_NOLOAD){
> + pr_debug("Invalid image type: %d\n", h->type);
> + return -EINVAL;
> + }
> +
> + if (h->arch != ARM_UIMAGE_ARCH_ARM) {
> + pr_debug("Invalidy image arch: %d\n", h->arch);
> + return -EINVAL;
> + }
> +
> + memcpy((char *)&uhdr, h, sizeof(uhdr));
> + crc = be32_to_cpu(uhdr.hdr_crc);
> + uhdr.hdr_crc = 0;
> +
> + if (crc32_ones(0, (char *)&uhdr, sizeof(uhdr)) != crc) {
> + pr_debug("Corrupt header, CRC do not match\n");
> + return -EINVAL;
> + }
> +
> + crc = be32_to_cpu(uhdr.crc);
> + if (crc32_ones(0, uimage_buf + zoff, uimage_len - zoff) != crc) {
> + pr_debug("Corrupt zImage, CRC do not match\n");
> + return -EINVAL;
> + }
> +
> + return kexec_zimage_ops.probe(uimage_buf + zoff,
> + uimage_len - zoff);
> +}
> +
> +static void *uimage_load(struct kimage *image,
> + char *uimage, unsigned long uimage_len,
> + char *initrd, unsigned long initrd_len,
> + char *cmdline, unsigned long cmdline_len)
> +{
> + const struct arm_uimage_header *h =
> + (struct arm_uimage_header *) uimage;
> + unsigned long zimage_offset = sizeof(struct arm_uimage_header);
> +
> + pr_debug("Loading uImage");
> + return kexec_zimage_ops.load(image,
> + uimage + zimage_offset,
> + uimage_len - zimage_offset,
> + initrd, initrd_len,
> + cmdline, cmdline_len);
> +}
> +
> +const struct kexec_file_ops kexec_uimage_ops = {
> + .probe = uimage_probe,
> + .load = uimage_load,
> +};
> diff --git a/arch/arm/kernel/kexec_zimage.c b/arch/arm/kernel/kexec_zimage.c
> new file mode 100644
> index 000000000000..d09795fc9072
> --- /dev/null
> +++ b/arch/arm/kernel/kexec_zimage.c
> @@ -0,0 +1,199 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Kexec zImage loader
> + *
> + * Copyright (C) 2020 Samsung Electronics
> + * Author: Łukasz Stelmach <[email protected]>

Please credit me as part author of this - you have taken some of my
code from the userspace kexec tool (such as the contents of
find_extension_tag()) and copied it in here, so this is not all your
own work.

> + */
> +
> +#define pr_fmt(fmt) "kexec_file(zImage): " fmt
> +
> +#include <asm/image.h>
> +#include <asm/unaligned.h>
> +#include <linux/err.h>
> +#include <linux/kexec.h>
> +#include <linux/memblock.h>
> +
> +#define byte_size(t) ((t)->hdr.size << 2)
> +
> +static const void *find_extension_tag(const char *buf,
> + unsigned long len,
> + uint32_t tag_id)
> +{
> + const struct arm_zimage_header *h = (const struct arm_zimage_header *)buf;
> + const struct arm_zimage_tag *tag;
> + uint32_t offset, size;
> + uint32_t max = len - sizeof(struct tag_header);
> +
> + if (len < sizeof(*h) ||
> + h->magic != ARM_ZIMAGE_MAGIC1 ||
> + h->magic2 != ARM_ZIMAGE_MAGIC2)
> + return NULL;
> +
> + for (offset = h->extension_tag_offset;
> + (tag = (void *)(buf + offset)) != NULL &&
> + offset < max &&
> + (size = le32_to_cpu(byte_size(tag))) != 0 &&
> + offset + size < len;
> + offset += size) {
> + pr_debug(" offset 0x%08x tag 0x%08x size %u\n",
> + offset, le32_to_cpu(tag->hdr.tag), size);
> + if (tag->hdr.tag == tag_id)
> + return tag;
> + }
> +
> + return NULL;
> +}
> +
> +static int zimage_probe(const char *kernel_buf, unsigned long kernel_len)
> +{
> + const struct arm_zimage_header *h =
> + (struct arm_zimage_header *)(kernel_buf);
> +
> + if (!h || (kernel_len < sizeof(*h)))
> + return -EINVAL;
> +
> + if ((h->magic != ARM_ZIMAGE_MAGIC1) ||
> + (h->magic2 != ARM_ZIMAGE_MAGIC2))
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +
> +#if defined(DEBUG)
> +#define debug_offsets() ({ \
> + pr_debug("Image offsets:\n"); \
> + pr_debug(" kernel 0x%08lx 0x%08lx\n", kernel_offset, kernel_len); \
> + pr_debug(" zimage 0x%08lx 0x%08lx\n", zimage_offset, zimage_len); \
> + pr_debug(" initrd 0x%08lx 0x%08lx\n", initrd_offset, initrd_len); \
> +})
> +#else
> +#define debug_offsets()
> +#endif
> +
> +static void *zimage_load(struct kimage *image,
> + char *zimage, unsigned long zimage_len,
> + char *initrd, unsigned long initrd_len,
> + char *cmdline, unsigned long cmdline_len)
> +{
> + struct arm_zimage_header *h;
> + struct kexec_buf kbuf;
> + struct kexec_segment *zimage_segment;
> + const struct arm_zimage_tag *klsz_tag;
> + const struct arm_zimage_tag_dc *dcsz_tag;
> + int ret = -EINVAL;
> +
> + unsigned long zimage_mem = 0x20000; /* malloc 64kB + stack 4 kB + some bss */
> + unsigned long kernel_len = zimage_len * 4; /* 4:1 compression */

This has been proven wrong.

> + unsigned long kernel_offset = memblock_start_of_DRAM() +
> + ALIGN(TEXT_OFFSET, PAGE_SIZE);

TEXT_OFFSET is actually a property of the loaded kernel, not of the
currently running kernel. I have a patch to add that into the zImage
tag.

> + unsigned long zimage_offset = kernel_offset +
> + ALIGN(kernel_len, PAGE_SIZE);
> + unsigned long initrd_offset = zimage_offset +
> + ALIGN(zimage_len + zimage_mem, PAGE_SIZE);

Since kernel_len is wrong, these will be wrong... please only fall
back to this if you don't find the extension tag - in other words,
declare the variables here, but don't initialise them, set them
lower down in the file if we fail to find the extension tag.

> +
> + if (image->type == KEXEC_TYPE_CRASH) {
> + kernel_offset += crashk_res.start;
> + zimage_offset += crashk_res.start;
> + initrd_offset += crashk_res.start;
> + }
> + debug_offsets();
> +
> + h = (struct arm_zimage_header *)zimage;
> +
> + klsz_tag = find_extension_tag(zimage, zimage_len, ZIMAGE_TAG_KRNL_SIZE);
> + if (klsz_tag) {
> + uint32_t *p = (void *)zimage +
> + le32_to_cpu(klsz_tag->u.krnl_size.size_ptr);
> + uint32_t edata_size = le32_to_cpu(get_unaligned(p));
> + uint32_t bss_size = le32_to_cpu(klsz_tag->u.krnl_size.bss_size);
> +
> + kernel_len = edata_size + bss_size;
> +
> + pr_debug("Decompressed kernel sizes:\n");
> + pr_debug(" text+data 0x%08lx bss 0x%08lx total 0x%08lx\n",
> + (unsigned long)edata_size,
> + (unsigned long)bss_size,
> + (unsigned long)kernel_len);
> +
> + zimage_offset = kernel_offset + ALIGN(edata_size, PAGE_SIZE);
> + initrd_offset = zimage_offset +
> + max(ALIGN(zimage_len + 0x20000, PAGE_SIZE),
> + ALIGN((unsigned long)bss_size, PAGE_SIZE));
> + debug_offsets();

This looks incorrect to me. Please see the kexec tool - what its doing
in its corresponding section that you copied some of this code from is
carefully thought out and can't be simplified. Ergo, I think this is
wrong.

> + }
> +
> + dcsz_tag = find_extension_tag(zimage, zimage_len,
> + ZIMAGE_TAG_DECOMP_SIZE);
> + if (dcsz_tag) {
> + uint32_t bss_size = le32_to_cpu(dcsz_tag->u.decomp_size.bss_size);
> + uint32_t stack_size = le32_to_cpu(dcsz_tag->u.decomp_size.stack_size);
> + uint32_t malloc_size = le32_to_cpu(dcsz_tag->u.decomp_size.malloc_size);
> +
> + zimage_mem = bss_size + stack_size + malloc_size;
> +
> + pr_debug("Decompressor memory requirements:\n");
> + pr_debug(" bss 0x%08lx stack 0x%08lx malloc 0x%08lx total 0x%08lx\n",
> + (unsigned long)bss_size,
> + (unsigned long)stack_size,
> + (unsigned long)malloc_size,
> + zimage_mem);
> +
> + initrd_offset = max(ALIGN(zimage_offset + zimage_len +
> + bss_size + stack_size +
> + malloc_size, PAGE_SIZE),
> + ALIGN(kernel_offset + kernel_len, PAGE_SIZE));
> + debug_offsets();
> + }
> +
> + /*
> + * zImage MUST be loaded into the first 128 MiB of physical
> + * memory for proper memory detection. Should the uncompressed
> + * kernel be larger than 128 MiB, zImage relocation becomes
> + * unavoidable and it is best to rely on the relocation code.
> + */
> + if (((zimage_offset - kernel_offset) + PAGE_SIZE + 0x8000) >= SZ_128M) {
> + pr_debug("The kernel is too big (%ld MiB) to avoid "
> + "zImage relocation. Loading zimage at 0x%08lx\n",
> + ((zimage_offset - kernel_offset) >> 20),
> + kernel_offset);
> + zimage_offset = kernel_offset;
> + }
> +
> + kbuf.image = image;
> + kbuf.top_down = false;
> +
> + kbuf.buf_min = zimage_offset;
> + kbuf.buf_max = ULONG_MAX;
> + kbuf.buffer = zimage;
> + kbuf.bufsz = zimage_len;
> + kbuf.buf_align = PAGE_SIZE;
> +
> + kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
> + kbuf.memsz = zimage_len;
> +
> + ret = kexec_add_buffer(&kbuf);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + pr_debug("Loaded zImage at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
> + kbuf.mem, kbuf.bufsz, kbuf.memsz);
> +
> + initrd_offset += kbuf.mem - zimage_offset;
> + debug_offsets();
> +
> + zimage_segment = &image->segment[image->nr_segments - 1];
> + image->start = zimage_segment->mem;
> +
> + ret = load_other_segments(image,
> + zimage_segment->mem, zimage_segment->memsz,
> + initrd, initrd_len, initrd_offset,
> + cmdline);
> + return ERR_PTR(ret);
> +}
> +
> +const struct kexec_file_ops kexec_zimage_ops = {
> + .probe = zimage_probe,
> + .load = zimage_load,
> +};
> diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c
> index c10a2dfd53d1..2e4780efabb4 100644
> --- a/arch/arm/kernel/machine_kexec.c
> +++ b/arch/arm/kernel/machine_kexec.c
> @@ -93,10 +93,13 @@ int machine_kexec_prepare(struct kimage *image)
> current_segment->memsz))
> return -EINVAL;
>
> - err = get_user(header, (__be32*)current_segment->buf);
> - if (err)
> - return err;
> -
> + if (image->file_mode) {
> + header = *(__be32 *)current_segment->buf;
> + } else {
> + err = get_user(header, (__be32 *)current_segment->buf);
> + if (err)
> + return err;
> + }
> if (header == cpu_to_be32(OF_DT_HEADER))
> image->arch.kernel_r2 = current_segment->mem;
> }
> diff --git a/arch/arm/kernel/machine_kexec_file.c b/arch/arm/kernel/machine_kexec_file.c
> new file mode 100644
> index 000000000000..ead680f1e795
> --- /dev/null
> +++ b/arch/arm/kernel/machine_kexec_file.c
> @@ -0,0 +1,209 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * kexec_file for arm
> + *
> + * Copyright (C) 2018 Linaro Limited
> + * Copyright (C) 2020 Samsung Electronics
> + * Authors:
> + * AKASHI Takahiro <[email protected]>
> + * Łukasz Stelmach <[email protected]>
> + *
> + */
> +
> +#define pr_fmt(fmt) "kexec_file: " fmt
> +
> +#include <linux/kexec.h>
> +#include <linux/libfdt.h>
> +#include <linux/of_fdt.h>
> +#include <linux/random.h>
> +
> +/* relevant device tree properties */
> +#define FDT_PROP_INITRD_START "linux,initrd-start"
> +#define FDT_PROP_INITRD_END "linux,initrd-end"
> +#define FDT_PROP_BOOTARGS "bootargs"
> +#define FDT_PROP_RNG_SEED "rng-seed"
> +
> +static int setup_dtb(struct kimage *image,
> + unsigned long initrd_load_addr, unsigned long initrd_len,
> + char *cmdline, void *dtb)
> +{
> + int off, ret;
> +
> + ret = fdt_path_offset(dtb, "/chosen");
> + if (ret < 0)
> + goto out;
> +
> + off = ret;
> +
> + /* add bootargs */
> + if (cmdline) {
> + ret = fdt_setprop_string(dtb, off, FDT_PROP_BOOTARGS, cmdline);
> + if (ret)
> + goto out;
> + } else {
> + ret = fdt_delprop(dtb, off, FDT_PROP_BOOTARGS);
> + if (ret && (ret != -FDT_ERR_NOTFOUND))
> + goto out;
> + }
> +
> + /* add initrd-* */
> + if (initrd_load_addr) {
> + ret = fdt_setprop_u64(dtb, off, FDT_PROP_INITRD_START,
> + initrd_load_addr);
> + if (ret)
> + goto out;
> +
> + ret = fdt_setprop_u64(dtb, off, FDT_PROP_INITRD_END,
> + initrd_load_addr + initrd_len);
> + if (ret)
> + goto out;
> + } else {
> + ret = fdt_delprop(dtb, off, FDT_PROP_INITRD_START);
> + if (ret && (ret != -FDT_ERR_NOTFOUND))
> + goto out;
> +
> + ret = fdt_delprop(dtb, off, FDT_PROP_INITRD_END);
> + if (ret && (ret != -FDT_ERR_NOTFOUND))
> + goto out;
> + }
> +
> + /* add rng-seed */
> + if (rng_is_initialized()) {
> + char seed[128];
> + get_random_bytes(seed, sizeof(seed));
> +
> + ret = fdt_setprop(dtb, off, FDT_PROP_RNG_SEED,
> + seed, sizeof(seed));
> + if (ret)
> + goto out;
> + } else {
> + pr_notice("RNG is not initialised: omitting \"%s\" property\n",
> + FDT_PROP_RNG_SEED);
> + ret = 0;
> + }
> +
> +out:
> + if (ret)
> + return (ret == -FDT_ERR_NOSPACE) ? -ENOMEM : -EINVAL;
> +
> + return 0;
> +}
> +/*
> + * More space needed so that we can add initrd, bootargs and kaslr-seed.
> + */
> +#define DTB_EXTRA_SPACE 0x1000
> +
> +static int create_dtb(struct kimage *image,
> + unsigned long initrd_load_addr, unsigned long initrd_len,
> + char *cmdline, void **dtb)
> +{
> + void *buf;
> + size_t buf_size;
> + size_t cmdline_len;
> + int ret;
> +
> + cmdline_len = cmdline ? strlen(cmdline) : 0;
> + buf_size = fdt_totalsize(initial_boot_params)
> + + cmdline_len + DTB_EXTRA_SPACE;
> +
> + for (;;) {
> + buf = vmalloc(buf_size);
> + if (!buf)
> + return -ENOMEM;
> +
> + /* duplicate a device tree blob */
> + ret = fdt_open_into(initial_boot_params, buf, buf_size);
> + if (ret)
> + return -EINVAL;
> +
> + ret = setup_dtb(image, initrd_load_addr, initrd_len,
> + cmdline, buf);
> + if (ret) {
> + vfree(buf);
> + if (ret == -ENOMEM) {
> + /* unlikely, but just in case */
> + buf_size += DTB_EXTRA_SPACE;
> + continue;
> + } else {
> + return ret;
> + }
> + }
> +
> + /* trim it */
> + fdt_pack(buf);
> + *dtb = buf;
> +
> + return 0;
> + }
> +}
> +
> +int load_other_segments(struct kimage *image,
> + unsigned long zimage_load_addr,
> + unsigned long zimage_len,
> + char *initrd,
> + unsigned long initrd_len,
> + unsigned long initrd_offset,
> + char *cmdline)
> +{
> + struct kexec_buf kbuf;
> + void *dtb = NULL;
> + unsigned long initrd_load_addr = 0;
> + unsigned long dtb_len;
> + int ret = 0;
> +
> + kbuf.image = image;
> + /* not allocate anything below the kernel */
> + kbuf.buf_min = initrd_offset;
> + if (initrd) {
> + kbuf.buffer = initrd;
> + kbuf.bufsz = initrd_len;
> + kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
> + kbuf.memsz = initrd_len;
> + kbuf.buf_align = PAGE_SIZE;
> + kbuf.buf_max = ULONG_MAX;
> + kbuf.top_down = false;
> +
> + ret = kexec_add_buffer(&kbuf);
> + if (ret)
> + goto out_err;
> +
> + pr_debug("Loaded initrd at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
> + kbuf.mem, kbuf.bufsz, kbuf.memsz);
> +
> + initrd_load_addr = kbuf.mem;
> + kbuf.buf_min = initrd_load_addr + kbuf.memsz;
> + }
> +
> + /* load dtb */
> + ret = create_dtb(image, initrd_load_addr, initrd_len, cmdline, &dtb);
> + if (ret) {
> + pr_err("Preparing for new dtb failed\n");
> + goto out_err;
> + }
> +
> + dtb_len = fdt_totalsize(dtb);
> + kbuf.buffer = dtb;
> + kbuf.bufsz = dtb_len;
> + kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
> + kbuf.memsz = dtb_len;
> + kbuf.buf_align = PAGE_SIZE;
> + kbuf.buf_max = ULONG_MAX;
> + kbuf.top_down = false;
> +
> + ret = kexec_add_buffer(&kbuf);
> + if (ret)
> + goto out_err;
> +
> + pr_debug("Loaded dtb at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
> + kbuf.mem, kbuf.bufsz, kbuf.memsz);
> + return 0;
> +out_err:
> + vfree(dtb);
> + return ret;
> +}
> +
> +const struct kexec_file_ops * const kexec_file_loaders[] = {
> + &kexec_uimage_ops,
> + &kexec_zimage_ops,
> + NULL
> +};
> --
> 2.26.2
>
>

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

2020-06-01 15:15:08

by Russell King (Oracle)

[permalink] [raw]

Subject: Re: [PATCH 1/5] arm: decompressor: set malloc pool size for the decompressor

On Mon, Jun 01, 2020 at 04:56:40PM +0200, Lukasz Stelmach wrote:
> It was <2020-06-01 pon 15:46>, when Russell King - ARM Linux admin wrote:
> > On Mon, Jun 01, 2020 at 04:27:50PM +0200, Łukasz Stelmach wrote:
> >> Move the definition of malloc pool size of the decompressor to
> >> a single place. This value will be exposed later for kexec_file loader.
> >>
> >> Signed-off-by: Łukasz Stelmach <[email protected]>
> >> ---
> >> arch/arm/boot/compressed/Makefile | 2 ++
> >> arch/arm/boot/compressed/head.S | 6 ++++--
> >> 2 files changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
> >> index 9c11e7490292..b3594cd1588c 100644
> >> --- a/arch/arm/boot/compressed/Makefile
> >> +++ b/arch/arm/boot/compressed/Makefile
> >> @@ -125,6 +125,8 @@ KBSS_SZ = $(shell echo $$(($$($(NM) $(obj)/../../../../vmlinux | \
> >> sed -n -e 's/^$[^ ]*$ [AB] __bss_start$$/-0x\1/p' \
> >> -e 's/^$[^ ]*$ [AB] __bss_stop$$/+0x\1/p') )) )
> >> LDFLAGS_vmlinux = --defsym _kernel_bss_size=$(KBSS_SZ)
> >> +# malloc pool size
> >> +LDFLAGS_vmlinux += --defsym _malloc_size=0x10000
> >> # Supply ZRELADDR to the decompressor via a linker symbol.
> >> ifneq ($(CONFIG_AUTO_ZRELADDR),y)
> >> LDFLAGS_vmlinux += --defsym zreladdr=$(ZRELADDR)
> >> diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
> >> index e8e1c866e413..dcc1afa60fb9 100644
> >> --- a/arch/arm/boot/compressed/head.S
> >> +++ b/arch/arm/boot/compressed/head.S
> >> @@ -309,7 +309,8 @@ restart: adr r0, LC0
> >> #ifndef CONFIG_ZBOOT_ROM
> >> /* malloc space is above the relocated stack (64k max) */
> >> add sp, sp, r0
> >> - add r10, sp, #0x10000
> >> + ldr r10, =_malloc_size
> >> + add r10, r10, sp
> >
> > This says "locate _malloc_size in a literal pool somewhere, and load it
> > using a PC-relative offset". Are you sure that the literal pool is
> > sensibly located?
> >
> > Would it be better to use a definition for this?
>
> I've followed ZRELADDR way. I think both values should be handled the
> same way (it makes it easier to comprehend IMHO). Is there any reason
> not to? Should I change ZRELADDR for v2 too?

There's a good reason. ZRELADDR can be a constant that does not fit
into the ARMs immediate constants (8 bits of significant data plus
a multiple of a 2-bit shift), whereas the size of the malloc space
is guaranteed to fit. So, ZRELADDR has to use a literal load.
This doesn't.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

2020-06-01 15:17:08

by Russell King (Oracle)

On Mon, Jun 01, 2020 at 06:19:52PM +0200, Lukasz Stelmach wrote:
> It was <2020-06-01 pon 15:55>, when Russell King - ARM Linux admin wrote:
> > On Mon, Jun 01, 2020 at 04:27:52PM +0200, Łukasz Stelmach wrote:
> >> Add DCSZ tag which holds dynamic memory (stack, bss, malloc pool)
> >> requirements of the decompressor code.
> >
> > Why do we need to know the stack and BSS size, when the userspace
> > kexec tool doesn't need to know this to perform the same function.
>
>
> kexec-tools load zImage as low in DRAM as possible and rely on two
> assumptions:
>
> + the zImage will copy itself to make enough room for the kernel,
> + sizeof(zImage+mem) < sizeof(kernel+mem), which is true because
> of compression.
>
> DRAM start
> + 0x8000
>
> zImage |-----------|-----|-------|
> text+data bss stack
>
> text+data bss
> kernel |---------------------|-------------------|
>
>
> initrd |-initrd-|-dtb-|

This is actually incorrect, because the decompressor will self-
relocate itself to avoid the area that it is going to decompress
into. So, while the decompressor runs, in the above situation it
ends up as:

ram |------------------------------------------------------...
text+data bss
kernel |---------------------|-------------------|
zImage |-----------|-----|-------|
text+data bss stack+malloc

Where "text+data" is actually smaller than the image size that
was loaded - the part of the image that performs the relocation
is discarded (the first chunk of code up to "restart" - 200
bytes.) The BSS is typically smaller than 200 bytes, so we've
been able to get away without knowing the actual BSS size so
far.

ram |--|-----------------------------------------|---------...
|<>| TEXT_OFFSET
kernel |---------------------|-------------------|
|<----edata_size----->|<-----bss_size---->|
|<---------------kernel_size------------->|
zImage |-----------|-----|-------|
|<-------len------->| (initial)
|<----------len------------>| (final)

The "final" len value is what the decompressor prints as the "zImage
requires" debugging value.

Hence, the size that the decompressed kernel requires is kernel_size.

The size that the decompressor requires is edata_size + len(final).

Now, if you intend to load the kernel to ram + TEXT_OFFSET + edata_size
then it isn't going to lose the first 200 bytes of code, so as you
correctly point out, we need to know the BSS size.

> >> +struct arm_zimage_tag_dc {
> >> + struct tag_header hdr;
> >> + union {
> >> +#define ZIMAGE_TAG_DECOMP_SIZE ARM_ZIMAGE_MAGIC4
> >> + struct zimage_decomp_size {
> >> + __le32 bss_size;
> >> + __le32 stack_size;
> >> + __le32 malloc_size;
> >> + } decomp_size;

You certainly don't need to know all this. All you need to know is
how much space the decompressor requires after the end of the image,
encompassing the BSS size, stack size and malloc size, which is one
value.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

2020-06-01 18:52:53

by Łukasz Stelmach

[permalink] [raw]

Subject: Re: [PATCH 5/5] arm: kexec_file: load zImage or uImage, initrd and dtb

It was <2020-06-01 pon 16:07>, when Russell King - ARM Linux admin wrote:
> On Mon, Jun 01, 2020 at 04:27:54PM +0200, Łukasz Stelmach wrote:
>> This is kexec_file_load implementation for ARM. It loads zImage and
>> initrd from file descripters and resuses DTB.
>>
>> Most code is derived from arm64 kexec_file_load implementation
>> and from kexec-tools.
>
> Please make the uImage loader able to be configured out of the
> kernel; it's a legacy image format now, and some of us just don't
> use it anymore. That way, those who wish to have it can, and
> those who wish not to can have a smaller kernel.

The difference in size of arch/arm/boot/Image is 160 bytes, but
OK. Done.

>> Cc: AKASHI Takahiro <[email protected]>
>> Signed-off-by: Łukasz Stelmach <[email protected]>
>> ---
>> arch/arm/Kconfig | 15 ++
>> arch/arm/include/asm/image.h | 26 ++++
>> arch/arm/include/asm/kexec.h | 14 ++
>> arch/arm/kernel/Makefile | 5 +-
>> arch/arm/kernel/kexec_uimage.c | 80 ++++++++++
>> arch/arm/kernel/kexec_zimage.c | 199 +++++++++++++++++++++++++
>> arch/arm/kernel/machine_kexec.c | 11 +-
>> arch/arm/kernel/machine_kexec_file.c | 209 +++++++++++++++++++++++++++
>> 8 files changed, 554 insertions(+), 5 deletions(-)
>> create mode 100644 arch/arm/kernel/kexec_uimage.c
>> create mode 100644 arch/arm/kernel/kexec_zimage.c
>> create mode 100644 arch/arm/kernel/machine_kexec_file.c
>>
>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>> index c77c93c485a0..6adb849cb304 100644
>> --- a/arch/arm/Kconfig
>> +++ b/arch/arm/Kconfig
>> @@ -1917,6 +1917,21 @@ config KEXEC
>> is properly shutdown, so do not be surprised if this code does not
>> initially work for you.
>>
>> +config KEXEC_FILE
>> + bool "Kexec file based system call (EXPERIMENTAL)"
>> + depends on (!SMP || PM_SLEEP_SMP)
>> + depends on USE_OF
>> + select KEXEC_CORE
>> + select CRC32
>> + help
>> + This is new version of kexec system call. This system call is
>> + file based and takes file descriptors as system call argument
>> + for kernel and initramfs as opposed to list of segments as
>> + accepted by previous system call.
>> +
>> + The kernel to be loaded MUST support Flattened Device Tree
>> + (selected with CONFIG_USE_OF).
>> +
>> config ATAGS_PROC
>> bool "Export atags in procfs"
>> depends on ATAGS && KEXEC
>> diff --git a/arch/arm/include/asm/image.h b/arch/arm/include/asm/image.h
>> index 624438740f23..95f23837b04f 100644
>> --- a/arch/arm/include/asm/image.h
>> +++ b/arch/arm/include/asm/image.h
>> @@ -8,8 +8,13 @@
>> (((x) >> 8) & 0x0000ff00) | \
>> (((x) << 8) & 0x00ff0000) | \
>> (((x) << 24) & 0xff000000))
>> +#define UIMAGE_MAGIC(x) (x)
>> #else
>> #define ZIMAGE_MAGIC(x) (x)
>> +#define UIMAGE_MAGIC(x) ((((x) >> 24) & 0x000000ff) | \
>> + (((x) >> 8) & 0x0000ff00) | \
>> + (((x) << 8) & 0x00ff0000) | \
>> + (((x) << 24) & 0xff000000))
>> #endif
>>
>> #define ARM_ZIMAGE_MAGIC1 ZIMAGE_MAGIC(0x016f2818)
>> @@ -17,6 +22,12 @@
>> #define ARM_ZIMAGE_MAGIC3 ZIMAGE_MAGIC(0x5a534c4b)
>> #define ARM_ZIMAGE_MAGIC4 ZIMAGE_MAGIC(0x5a534344)
>>
>> +#define ARM_UIMAGE_MAGIC UIMAGE_MAGIC(0x27051956)
>> +#define ARM_UIMAGE_NAME_LEN 32
>> +#define ARM_UIMAGE_TYPE_KERNEL 2
>> +#define ARM_UIMAGE_TYPE_KERNEL_NOLOAD 14
>> +#define ARM_UIMAGE_ARCH_ARM 2
>> +
>> #ifndef __ASSEMBLY__
>>
>> #include <linux/types.h>
>> @@ -33,6 +44,21 @@ struct arm_zimage_header {
>> __le32 extension_tag_offset;
>> };
>>
>> +struct arm_uimage_header {
>> + __be32 magic;
>> + __be32 hdr_crc;
>> + __be32 time;
>> + __be32 size;
>> + __be32 load;
>> + __be32 entry;
>> + __be32 crc;
>> + __u8 os;
>> + __u8 arch;
>> + __u8 type;
>> + __u8 comp;
>> + __u8 name[ARM_UIMAGE_NAME_LEN];
>> +};
>> +
>> struct arm_zimage_tag {
>> struct tag_header hdr;
>> union {
>> diff --git a/arch/arm/include/asm/kexec.h b/arch/arm/include/asm/kexec.h
>> index 22751b5b5735..fda35afa7195 100644
>> --- a/arch/arm/include/asm/kexec.h
>> +++ b/arch/arm/include/asm/kexec.h
>> @@ -83,6 +83,20 @@ static inline struct page *boot_pfn_to_page(unsigned long boot_pfn)
>> }
>> #define boot_pfn_to_page boot_pfn_to_page
>>
>> +#ifdef CONFIG_KEXEC_FILE
>> +
>> +extern const struct kexec_file_ops kexec_zimage_ops;
>> +extern const struct kexec_file_ops kexec_uimage_ops;
>> +
>> +struct kimage;
>> +
>> +extern int load_other_segments(struct kimage *image,
>> + unsigned long kernel_load_addr, unsigned long kernel_size,
>> + char *initrd, unsigned long initrd_len,
>> + unsigned long initrd_offset, char *cmdline);
>> +
>> +#endif /* CONFIG_KEXEC_FILE */
>> +
>> #endif /* __ASSEMBLY__ */
>>
>> #endif /* CONFIG_KEXEC */
>> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
>> index 89e5d864e923..466c683bb551 100644
>> --- a/arch/arm/kernel/Makefile
>> +++ b/arch/arm/kernel/Makefile
>> @@ -3,6 +3,7 @@
>> # Makefile for the linux kernel.
>> #
>>
>> +CFLAGS_kexec_zimage.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
>> CPPFLAGS_vmlinux.lds := -DTEXT_OFFSET=$(TEXT_OFFSET)
>> AFLAGS_head.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
>>
>> @@ -56,7 +57,9 @@ obj-$(CONFIG_FUNCTION_TRACER) += entry-ftrace.o
>> obj-$(CONFIG_DYNAMIC_FTRACE) += ftrace.o insn.o patch.o
>> obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o insn.o patch.o
>> obj-$(CONFIG_JUMP_LABEL) += jump_label.o insn.o patch.o
>> -obj-$(CONFIG_KEXEC) += machine_kexec.o relocate_kernel.o
>> +obj-$(CONFIG_KEXEC_CORE) += machine_kexec.o relocate_kernel.o
>> +obj-$(CONFIG_KEXEC_FILE) += machine_kexec_file.o kexec_zimage.o \
>> + kexec_uimage.o
>> # Main staffs in KPROBES are in arch/arm/probes/ .
>> obj-$(CONFIG_KPROBES) += patch.o insn.o
>> obj-$(CONFIG_OABI_COMPAT) += sys_oabi-compat.o
>> diff --git a/arch/arm/kernel/kexec_uimage.c b/arch/arm/kernel/kexec_uimage.c
>> new file mode 100644
>> index 000000000000..47033574e24e
>> --- /dev/null
>> +++ b/arch/arm/kernel/kexec_uimage.c
>> @@ -0,0 +1,80 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Kexec uImage loader
>> + *
>> + * Copyright (C) 2020 Samsung Electronics
>> + * Author: Łukasz Stelmach <[email protected]>
>> + */
>> +
>> +#define pr_fmt(fmt) "kexec_file(uImage): " fmt
>> +
>> +#include <asm/image.h>
>> +#include <linux/crc32.h>
>> +#include <linux/err.h>
>> +#include <linux/kexec.h>
>> +
>> +#define crc32_ones(crc, buf, len) \
>> + (crc32(crc ^ 0xffffffff, buf, len) ^ 0xffffffff)
>> +
>> +static int uimage_probe(const char *uimage_buf, unsigned long uimage_len)
>> +{
>> + const struct arm_uimage_header *h =
>> + (struct arm_uimage_header *) uimage_buf;
>> + struct arm_uimage_header uhdr;
>> + unsigned long zoff = sizeof(struct arm_uimage_header);
>> + uint32_t crc;
>> +
>> + if (h->magic != ARM_UIMAGE_MAGIC)
>> + return -EINVAL;
>> +
>> + if (h->type != ARM_UIMAGE_TYPE_KERNEL &&
>> + h->type != ARM_UIMAGE_TYPE_KERNEL_NOLOAD){
>> + pr_debug("Invalid image type: %d\n", h->type);
>> + return -EINVAL;
>> + }
>> +
>> + if (h->arch != ARM_UIMAGE_ARCH_ARM) {
>> + pr_debug("Invalidy image arch: %d\n", h->arch);
>> + return -EINVAL;
>> + }
>> +
>> + memcpy((char *)&uhdr, h, sizeof(uhdr));
>> + crc = be32_to_cpu(uhdr.hdr_crc);
>> + uhdr.hdr_crc = 0;
>> +
>> + if (crc32_ones(0, (char *)&uhdr, sizeof(uhdr)) != crc) {
>> + pr_debug("Corrupt header, CRC do not match\n");
>> + return -EINVAL;
>> + }
>> +
>> + crc = be32_to_cpu(uhdr.crc);
>> + if (crc32_ones(0, uimage_buf + zoff, uimage_len - zoff) != crc) {
>> + pr_debug("Corrupt zImage, CRC do not match\n");
>> + return -EINVAL;
>> + }
>> +
>> + return kexec_zimage_ops.probe(uimage_buf + zoff,
>> + uimage_len - zoff);
>> +}
>> +
>> +static void *uimage_load(struct kimage *image,
>> + char *uimage, unsigned long uimage_len,
>> + char *initrd, unsigned long initrd_len,
>> + char *cmdline, unsigned long cmdline_len)
>> +{
>> + const struct arm_uimage_header *h =
>> + (struct arm_uimage_header *) uimage;
>> + unsigned long zimage_offset = sizeof(struct arm_uimage_header);
>> +
>> + pr_debug("Loading uImage");
>> + return kexec_zimage_ops.load(image,
>> + uimage + zimage_offset,
>> + uimage_len - zimage_offset,
>> + initrd, initrd_len,
>> + cmdline, cmdline_len);
>> +}
>> +
>> +const struct kexec_file_ops kexec_uimage_ops = {
>> + .probe = uimage_probe,
>> + .load = uimage_load,
>> +};
>> diff --git a/arch/arm/kernel/kexec_zimage.c b/arch/arm/kernel/kexec_zimage.c
>> new file mode 100644
>> index 000000000000..d09795fc9072
>> --- /dev/null
>> +++ b/arch/arm/kernel/kexec_zimage.c
>> @@ -0,0 +1,199 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Kexec zImage loader
>> + *
>> + * Copyright (C) 2020 Samsung Electronics
>> + * Author: Łukasz Stelmach <[email protected]>
>
> Please credit me as part author of this - you have taken some of my
> code from the userspace kexec tool (such as the contents of
> find_extension_tag()) and copied it in here, so this is not all your
> own work.

Done.

>> + */
>> +
>> +#define pr_fmt(fmt) "kexec_file(zImage): " fmt
>> +
>> +#include <asm/image.h>
>> +#include <asm/unaligned.h>
>> +#include <linux/err.h>
>> +#include <linux/kexec.h>
>> +#include <linux/memblock.h>
>> +
>> +#define byte_size(t) ((t)->hdr.size << 2)
>> +
>> +static const void *find_extension_tag(const char *buf,
>> + unsigned long len,
>> + uint32_t tag_id)
>> +{
>> + const struct arm_zimage_header *h = (const struct arm_zimage_header *)buf;
>> + const struct arm_zimage_tag *tag;
>> + uint32_t offset, size;
>> + uint32_t max = len - sizeof(struct tag_header);
>> +
>> + if (len < sizeof(*h) ||
>> + h->magic != ARM_ZIMAGE_MAGIC1 ||
>> + h->magic2 != ARM_ZIMAGE_MAGIC2)
>> + return NULL;
>> +
>> + for (offset = h->extension_tag_offset;
>> + (tag = (void *)(buf + offset)) != NULL &&
>> + offset < max &&
>> + (size = le32_to_cpu(byte_size(tag))) != 0 &&
>> + offset + size < len;
>> + offset += size) {
>> + pr_debug(" offset 0x%08x tag 0x%08x size %u\n",
>> + offset, le32_to_cpu(tag->hdr.tag), size);
>> + if (tag->hdr.tag == tag_id)
>> + return tag;
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +static int zimage_probe(const char *kernel_buf, unsigned long kernel_len)
>> +{
>> + const struct arm_zimage_header *h =
>> + (struct arm_zimage_header *)(kernel_buf);
>> +
>> + if (!h || (kernel_len < sizeof(*h)))
>> + return -EINVAL;
>> +
>> + if ((h->magic != ARM_ZIMAGE_MAGIC1) ||
>> + (h->magic2 != ARM_ZIMAGE_MAGIC2))
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
>> +
>> +
>> +#if defined(DEBUG)
>> +#define debug_offsets() ({ \
>> + pr_debug("Image offsets:\n"); \
>> + pr_debug(" kernel 0x%08lx 0x%08lx\n", kernel_offset, kernel_len); \
>> + pr_debug(" zimage 0x%08lx 0x%08lx\n", zimage_offset, zimage_len); \
>> + pr_debug(" initrd 0x%08lx 0x%08lx\n", initrd_offset, initrd_len); \
>> +})
>> +#else
>> +#define debug_offsets()
>> +#endif
>> +
>> +static void *zimage_load(struct kimage *image,
>> + char *zimage, unsigned long zimage_len,
>> + char *initrd, unsigned long initrd_len,
>> + char *cmdline, unsigned long cmdline_len)
>> +{
>> + struct arm_zimage_header *h;
>> + struct kexec_buf kbuf;
>> + struct kexec_segment *zimage_segment;
>> + const struct arm_zimage_tag *klsz_tag;
>> + const struct arm_zimage_tag_dc *dcsz_tag;
>> + int ret = -EINVAL;
>> +
>> + unsigned long zimage_mem = 0x20000; /* malloc 64kB + stack 4 kB + some bss */
>> + unsigned long kernel_len = zimage_len * 4; /* 4:1 compression */
>
> This has been proven wrong.

5:1? The comment and the code in kexec-tools are inconsisten in this
regard. The comment says 4:1, but the code multiplies by 5. Your
recommendation? This value is used as a fallback anyway in a very
unlikely case, when tags are missing (see below).

>> + unsigned long kernel_offset = memblock_start_of_DRAM() +
>> + ALIGN(TEXT_OFFSET, PAGE_SIZE);
>
> TEXT_OFFSET is actually a property of the loaded kernel, not of the
> currently running kernel.

Indeed.

> I have a patch to add that into the zImage tag.

I am afraid, I haven't seen it before. Where can I find it?

>> + unsigned long zimage_offset = kernel_offset +
>> + ALIGN(kernel_len, PAGE_SIZE);
>> + unsigned long initrd_offset = zimage_offset +
>> + ALIGN(zimage_len + zimage_mem, PAGE_SIZE);
>
> Since kernel_len is wrong, these will be wrong... please only fall
> back to this if you don't find the extension tag - in other words,
> declare the variables here, but don't initialise them, set them
> lower down in the file if we fail to find the extension tag.

That is how it works now. This is the structure I have taken from
kexec-tools: initialize and change if additional information is
available. If the tags are not available use these values as a fallback.

>> +
>> + if (image->type == KEXEC_TYPE_CRASH) {
>> + kernel_offset += crashk_res.start;
>> + zimage_offset += crashk_res.start;
>> + initrd_offset += crashk_res.start;
>> + }
>> + debug_offsets();
>> +
>> + h = (struct arm_zimage_header *)zimage;
>> +
>> + klsz_tag = find_extension_tag(zimage, zimage_len, ZIMAGE_TAG_KRNL_SIZE);
>> + if (klsz_tag) {
>> + uint32_t *p = (void *)zimage +
>> + le32_to_cpu(klsz_tag->u.krnl_size.size_ptr);
>> + uint32_t edata_size = le32_to_cpu(get_unaligned(p));
>> + uint32_t bss_size = le32_to_cpu(klsz_tag->u.krnl_size.bss_size);
>> +
>> + kernel_len = edata_size + bss_size;
>> +
>> + pr_debug("Decompressed kernel sizes:\n");
>> + pr_debug(" text+data 0x%08lx bss 0x%08lx total 0x%08lx\n",
>> + (unsigned long)edata_size,
>> + (unsigned long)bss_size,
>> + (unsigned long)kernel_len);
>> +
>> + zimage_offset = kernel_offset + ALIGN(edata_size, PAGE_SIZE);
>> + initrd_offset = zimage_offset +
>> + max(ALIGN(zimage_len + 0x20000, PAGE_SIZE),
>> + ALIGN((unsigned long)bss_size, PAGE_SIZE));
>> + debug_offsets();
>
> This looks incorrect to me. Please see the kexec tool - what its doing
> in its corresponding section that you copied some of this code from is
> carefully thought out and can't be simplified. Ergo, I think this is
> wrong.

As I explained in <dleftjwo4qsqqf.fsf%[email protected]> my
approach is slightly different. I am trying to avoid copying zImage by
putting it hight enough in the first place.

Kind regards,
--
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

Attachments:

signature.asc (497.00 B)

2020-06-01 20:30:12

It was <2020-06-01 pon 21:41>, when Russell King - ARM Linux admin wrote:
> On Mon, Jun 01, 2020 at 10:27:45PM +0200, Lukasz Stelmach wrote:
>> It was <2020-06-01 pon 19:25>, when Russell King - ARM Linux admin wrote:
>>> On Mon, Jun 01, 2020 at 06:19:52PM +0200, Lukasz Stelmach wrote:
>>>> It was <2020-06-01 pon 15:55>, when Russell King - ARM Linux admin wrote:
>>>>> On Mon, Jun 01, 2020 at 04:27:52PM +0200, Łukasz Stelmach wrote:
>>>>>> Add DCSZ tag which holds dynamic memory (stack, bss, malloc pool)
>>>>>> requirements of the decompressor code.
>>>>>
>>>>> Why do we need to know the stack and BSS size, when the userspace
>>>>> kexec tool doesn't need to know this to perform the same function.
>>>>
>>>>
>>>> kexec-tools load zImage as low in DRAM as possible and rely on two
>>>> assumptions:
>>>>
>>>> + the zImage will copy itself to make enough room for the kernel,
>>>> + sizeof(zImage+mem) < sizeof(kernel+mem), which is true because
>>>> of compression.
>>>>
>>>> DRAM start
>>>> + 0x8000
>>>>
>>>> zImage |-----------|-----|-------|
>>>> text+data bss stack
>>>>
>>>> text+data bss
>>>> kernel |---------------------|-------------------|
>>>>
>>>>
>>>> initrd |-initrd-|-dtb-|
>>>
>>> This is actually incorrect, because the decompressor will self-
>>> relocate itself to avoid the area that it is going to decompress
>>> into.
>>
>> I described the state right after kexec(8) invocation.
>
> Actually, you haven't, because this is _not_ how kexec(8) lays it
> out, as I attempted to detail further down in my reply.

Let me try to describe how I understand the code in kexec-tools
(commit 74c7c369).

--8<---------------cut here---------------start------------->8---
int zImage_arm_load(…, const char *buf, off_t len, …)
// buf - zImage
// len - size of zImage

unsigned int extra_size = 0x8000; /* TEXT_OFFSET */

kernel_mem_size = len + 4;

// locate a hole to fit zImage + 32kB as low as possible,
base = locate_hole(info, len + extra_size, 0, 0, ULONG_MAX, INT_MAX);

kernel_base = base + extra_size;

add_segment(info, buf, len, kernel_base, kernel_mem_size);
--8<---------------cut here---------------end--------------->8---

Therefore, zImage is loaded low and always requires relocation.

ram |--------------------------------------------------------------
zImage |----k_m_s----|
^
|
kernel_base — TEXT_OFFSET or higher

Next goes initrd

--8<---------------cut here---------------start------------->8---
kexec_arm_image_size = len * 5; // or passed on command line

// if the tag exists
kexec_arm_image_size = max(edata_size + bss_size,
edata_size + len); // len - zImage size + 64 kB

initrd_base = kernel_base + _ALIGN(kexec_arm_image_size, page_size);

add_segment(info, ramdisk_buf, initrd_size, initrd_base, initrd_size);
--8<---------------cut here---------------end--------------->8---

above whatever is bigger (kernel + kernel bss) OR (kernel + zImage + zImage mem).

ram |---------------------------------------------------------------
zImage |----k_m_s----| Where kexec loads zImage @kernel_base

|.............len * 5....................| Fallback
kernel |.....edata.....|...bss...| These are just calculations
zImage |.....len+....| zImage will copy itself here WHEN it runs

initrd |--initrd_size--|
dtb ^ |---|
|
initrd_base

DTB, of course, goes next

dtb_offset = initrd_base + initrd_size + page_size;

Stuff marked with "-" is actually loaded, "." are just calculations to
establish initrd_base.

>>> So, while the decompressor runs, in the above situation it
>>> ends up as:
>>>
>>>
>>> ram |------------------------------------------------------...
>>> text+data bss
>>> kernel |---------------------|-------------------|
>>> zImage |-----------|-----|-------|
>>> text+data bss stack+malloc
>
> Note here - if the initrd was placed as _you_ describe at the end
> of where the zImage ends up including its BSS, it would be
> corrupted by the stack and malloc space of the decompressor while
> running. Ergo, your description of how kexec(8) lays stuff out
> is incorrect.

Is my analysis above accurate now? Do I understand this?

As you noted, my intention is to load zImage after edata (dotted len+
above).

>>>>>> +struct arm_zimage_tag_dc {
>>>>>> + struct tag_header hdr;
>>>>>> + union {
>>>>>> +#define ZIMAGE_TAG_DECOMP_SIZE ARM_ZIMAGE_MAGIC4
>>>>>> + struct zimage_decomp_size {
>>>>>> + __le32 bss_size;
>>>>>> + __le32 stack_size;
>>>>>> + __le32 malloc_size;
>>>>>> + } decomp_size;
>>>
>>> You certainly don't need to know all this. All you need to know is
>>> how much space the decompressor requires after the end of the image,
>>> encompassing the BSS size, stack size and malloc size, which is one
>>> value.
>>
>> I agree. However, since we are not fighting here for every single byte,
>> I'd rather add them as separate values and make the tag, even if only
>> slightly, more future-proof.
>
> It doesn't make it more future-proof. What happens if we add something
> else, do we need to list it separately, and then change the kernel to
> accept the new value and maybe also kexec(8)? Or do we just say "the
> decompressor needs X many bytes after the image" and be done with it?
> The latter sounds way more future-proof to me.

You are right. I changed it to a single value. Done.

--
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

Attachments:

signature.asc (497.00 B)

2020-06-02 16:23:18

by Łukasz Stelmach

[permalink] [raw]

Subject: [PATCH v2 0/5] kexec_file_load() for arm

The following series of patches provides implementation of the
kexec_file_load() system call form the arm architecture. zImage and uImage
(legacy format) files are supported. Like on arm64, there is no
possibility of loading a new DTB and the currently loaded is reused.

Changes in v2:
- add CONFIG_KEXEC_FILE_UIMAGE for optional uImage support
- MALLOC_SIZE as a define instead of a symbol
- DCSZ tag holds combined dynamic memory requirements (bss+stack+malloc)
- use union for a single tag structure
- copyright notice includes Russell King

Łukasz Stelmach (5):
arm: decompressor: set malloc pool size for the decompressor
arm: add image header definitions
arm: decompressor: define a new zImage tag
arm: Add kexec_image_info
arm: kexec_file: load zImage or uImage, initrd and dtb

arch/arm/Kconfig | 25 +++
arch/arm/boot/compressed/Makefile | 7 +-
arch/arm/boot/compressed/head.S | 9 +-
arch/arm/boot/compressed/vmlinux.lds.S | 22 +--
arch/arm/include/asm/image.h | 77 +++++++++
arch/arm/include/asm/kexec.h | 14 ++
arch/arm/kernel/Makefile | 5 +-
arch/arm/kernel/kexec_uimage.c | 80 ++++++++++
arch/arm/kernel/kexec_zimage.c | 194 +++++++++++++++++++++++
arch/arm/kernel/machine_kexec.c | 39 ++++-
arch/arm/kernel/machine_kexec_file.c | 211 +++++++++++++++++++++++++
11 files changed, 662 insertions(+), 21 deletions(-)
create mode 100644 arch/arm/include/asm/image.h
create mode 100644 arch/arm/kernel/kexec_uimage.c
create mode 100644 arch/arm/kernel/kexec_zimage.c
create mode 100644 arch/arm/kernel/machine_kexec_file.c

--
2.26.2

2020-06-02 16:23:41

by Łukasz Stelmach

[permalink] [raw]

Hi "Łukasz,

I love your patch! Perhaps something to improve:

[auto build test WARNING on next-20200930]
[cannot apply to arm/for-next v5.9-rc7 v5.9-rc6 v5.9-rc5 v5.9-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/0day-ci/linux/commits/ukasz-Stelmach/kexec_file_load-for-arm/20201001-024045
base: de69ee6df1cfbf3c67787d8504fd21b59da39572
config: arm-allyesconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/9a7741b76697140672aba84338463032c1fc9fb8
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review ukasz-Stelmach/kexec_file_load-for-arm/20201001-024045
git checkout 9a7741b76697140672aba84338463032c1fc9fb8
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

arch/arm/kernel/kexec_zimage.c: In function 'zimage_load':
>> arch/arm/kernel/kexec_zimage.c:89:28: warning: variable 'h' set but not used [-Wunused-but-set-variable]
89 | struct arm_zimage_header *h;
| ^

vim +/h +89 arch/arm/kernel/kexec_zimage.c

83
84 static void *zimage_load(struct kimage *image,
85 char *zimage, unsigned long zimage_len,
86 char *initrd, unsigned long initrd_len,
87 char *cmdline, unsigned long cmdline_len)
88 {
> 89 struct arm_zimage_header *h;
90 struct kexec_buf kbuf;
91 struct kexec_segment *zimage_segment;
92 const struct arm_zimage_tag *tag;
93 int ret = -EINVAL;
94
95 unsigned long zimage_mem = 0x20000; /* malloc 64kB + stack 4 kB + some bss */
96 unsigned long kernel_len = zimage_len * 5; /* 5:1 compression */
97 unsigned long kernel_offset = memblock_start_of_DRAM();
98 unsigned long zimage_offset = kernel_offset +
99 ALIGN(kernel_len, PAGE_SIZE);
100 unsigned long initrd_offset = zimage_offset +
101 ALIGN(zimage_len + zimage_mem, PAGE_SIZE);
102
103 if (image->type == KEXEC_TYPE_CRASH) {
104 kernel_offset += crashk_res.start;
105 zimage_offset += crashk_res.start;
106 initrd_offset += crashk_res.start;
107 }
108 debug_offsets();
109
110 h = (struct arm_zimage_header *)zimage;
111
112 tag = find_extension_tag(zimage, zimage_len, ZIMAGE_TAG_KRNL_SIZE);
113 if (tag) {
114 uint32_t *p = (void *)zimage +
115 le32_to_cpu(tag->u.krnl_size.size_ptr);
116 uint32_t edata_size = le32_to_cpu(get_unaligned(p));
117 uint32_t bss_size = le32_to_cpu(tag->u.krnl_size.bss_size);
118 uint32_t text_offset = le32_to_cpu(tag->u.krnl_size.text_offset);
119
120 kernel_offset += ALIGN(text_offset, PAGE_SIZE);
121 kernel_len = edata_size + bss_size;
122
123 pr_debug("Decompressed kernel sizes:\n");
124 pr_debug(" text+data 0x%08lx bss 0x%08lx total 0x%08lx\n",
125 (unsigned long)edata_size,
126 (unsigned long)bss_size,
127 (unsigned long)kernel_len);
128
129 zimage_offset = kernel_offset + ALIGN(edata_size, PAGE_SIZE);
130 initrd_offset = zimage_offset +
131 max(ALIGN(zimage_len + 0x20000, PAGE_SIZE),
132 ALIGN((unsigned long)bss_size, PAGE_SIZE));
133 debug_offsets();
134 }
135
136 tag = find_extension_tag(zimage, zimage_len,
137 ZIMAGE_TAG_ZIMAGE_MEM);
138 if (tag) {
139 uint32_t zimage_mem = le32_to_cpu(tag->u.zimage_mem);
140
141 pr_debug("Decompressor requires %d bytes of memory\n", zimage_mem);
142
143 initrd_offset = max(ALIGN(zimage_offset + zimage_len + zimage_mem, PAGE_SIZE),
144 ALIGN(kernel_offset + kernel_len, PAGE_SIZE));
145 debug_offsets();
146 }
147
148 /*
149 * zImage MUST be loaded into the first 128 MiB of physical
150 * memory for proper memory detection. Should the uncompressed
151 * kernel be larger than 128 MiB, zImage relocation becomes
152 * unavoidable and it is best to rely on the relocation code.
153 */
154 if (((zimage_offset - kernel_offset) + PAGE_SIZE + 0x8000) >= SZ_128M) {
155 pr_debug("The kernel is too big (%ld MiB) to avoid "
156 "zImage relocation. Loading zimage at 0x%08lx\n",
157 ((zimage_offset - kernel_offset) >> 20),
158 kernel_offset);
159 zimage_offset = kernel_offset;
160 }
161
162 kbuf.image = image;
163 kbuf.top_down = false;
164
165 kbuf.buf_min = zimage_offset;
166 kbuf.buf_max = ULONG_MAX;
167 kbuf.buffer = zimage;
168 kbuf.bufsz = zimage_len;
169 kbuf.buf_align = PAGE_SIZE;
170
171 kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
172 kbuf.memsz = zimage_len;
173
174 ret = kexec_add_buffer(&kbuf);
175 if (ret)
176 return ERR_PTR(ret);
177
178 pr_debug("Loaded zImage at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
179 kbuf.mem, kbuf.bufsz, kbuf.memsz);
180
181 initrd_offset += kbuf.mem - zimage_offset;
182 debug_offsets();
183
184 zimage_segment = &image->segment[image->nr_segments - 1];
185 image->start = zimage_segment->mem;
186
187 ret = load_other_segments(image,
188 zimage_segment->mem, zimage_segment->memsz,
189 initrd, initrd_len, initrd_offset,
190 cmdline);
191 return ERR_PTR(ret);
192 }
193

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]

Attachments:

(No filename) (6.01 kB)
.config.gz (74.88 kB)
Download all attachments