[PATCH v14 06/16] of/fdt: add helper functions for handling properties

[PATCH v14 06/16] of/fdt: add helper functions for handling properties

[AKASHI Takahiro]

These functions will be used later to handle kexec-specific properties
in arm64's kexec_file implementation.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
---
 drivers/of/fdt.c       | 62 ++++++++++++++++++++++++++++++++++++++++--
 include/linux/of_fdt.h | 10 +++++--
 2 files changed, 68 insertions(+), 4 deletions(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 800ad252cf9c..dc960cea1355 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -25,6 +25,7 @@
 #include <linux/debugfs.h>
 #include <linux/serial_core.h>
 #include <linux/sysfs.h>
+#include <linux/types.h>
 
 #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
 #include <asm/page.h>
@@ -537,8 +538,8 @@ void *of_fdt_unflatten_tree(const unsigned long *blob,
 EXPORT_SYMBOL_GPL(of_fdt_unflatten_tree);
 
 /* Everything below here references initial_boot_params directly. */
-int __initdata dt_root_addr_cells;
-int __initdata dt_root_size_cells;
+int dt_root_addr_cells;
+int dt_root_size_cells;
 
 void *initial_boot_params;
 
@@ -1323,3 +1324,60 @@ late_initcall(of_fdt_raw_init);
 #endif
 
 #endif /* CONFIG_OF_EARLY_FLATTREE */
+
+bool of_fdt_cells_size_fitted(u64 base, u64 size)
+{
+	/* if *_cells >= 2, cells can hold 64-bit values anyway */
+	if ((dt_root_addr_cells == 1) && (base > U32_MAX))
+		return false;
+
+	if ((dt_root_size_cells == 1) && (size > U32_MAX))
+		return false;
+
+	return true;
+}
+
+size_t of_fdt_reg_cells_size(void)
+{
+	return (dt_root_addr_cells + dt_root_size_cells) * sizeof(u32);
+}
+
+#define FDT_ALIGN(x, a)	(((x) + (a) - 1) & ~((a) - 1))
+#define FDT_TAGALIGN(x)	(FDT_ALIGN((x), FDT_TAGSIZE))
+
+int fdt_prop_len(const char *prop_name, int len)
+{
+	return (strlen(prop_name) + 1) +
+		sizeof(struct fdt_property) +
+		FDT_TAGALIGN(len);
+}
+
+static void fill_property(void *buf, u64 val64, int cells)
+{
+	__be32 val32;
+
+	while (cells) {
+		val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
+		memcpy(buf, &val32, sizeof(val32));
+		buf += sizeof(val32);
+	}
+}
+
+int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
+						u64 addr, u64 size)
+{
+	char buf[sizeof(__be32) * 2 * 2];
+		/* assume dt_root_[addr|size]_cells <= 2 */
+	void *prop;
+	size_t buf_size;
+
+	buf_size = of_fdt_reg_cells_size();
+	prop = buf;
+
+	fill_property(prop, addr, dt_root_addr_cells);
+	prop += dt_root_addr_cells * sizeof(u32);
+
+	fill_property(prop, size, dt_root_size_cells);
+
+	return fdt_setprop(fdt, nodeoffset, name, buf, buf_size);
+}
diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
index b9cd9ebdf9b9..9615d6142578 100644
--- a/include/linux/of_fdt.h
+++ b/include/linux/of_fdt.h
@@ -37,8 +37,8 @@ extern void *of_fdt_unflatten_tree(const unsigned long *blob,
 				   struct device_node **mynodes);
 
 /* TBD: Temporary export of fdt globals - remove when code fully merged */
-extern int __initdata dt_root_addr_cells;
-extern int __initdata dt_root_size_cells;
+extern int dt_root_addr_cells;
+extern int dt_root_size_cells;
 extern void *initial_boot_params;
 
 extern char __dtb_start[];
@@ -108,5 +108,11 @@ static inline void unflatten_device_tree(void) {}
 static inline void unflatten_and_copy_device_tree(void) {}
 #endif /* CONFIG_OF_EARLY_FLATTREE */
 
+bool of_fdt_cells_size_fitted(u64 base, u64 size);
+size_t of_fdt_reg_cells_size(void);
+int fdt_prop_len(const char *prop_name, int len);
+int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
+						u64 addr, u64 size);
+
 #endif /* __ASSEMBLY__ */
 #endif /* _LINUX_OF_FDT_H */
-- 
2.18.0

[PATCH v14 07/16] arm64: add image head flag definitions

[AKASHI Takahiro]

Those image head's flags will be used later by kexec_file loader.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Acked-by: James Morse <james.morse@arm.com>
---
 arch/arm64/include/asm/boot.h | 15 +++++++++++++++
 arch/arm64/kernel/head.S      |  2 +-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/boot.h b/arch/arm64/include/asm/boot.h
index 355e552a9175..0bab7eed3012 100644
--- a/arch/arm64/include/asm/boot.h
+++ b/arch/arm64/include/asm/boot.h
@@ -5,6 +5,21 @@
 
 #include <asm/sizes.h>
 
+#define ARM64_MAGIC		"ARM\x64"
+
+#define HEAD_FLAG_BE_SHIFT		0
+#define HEAD_FLAG_PAGE_SIZE_SHIFT	1
+#define HEAD_FLAG_BE_MASK		0x1
+#define HEAD_FLAG_PAGE_SIZE_MASK	0x3
+
+#define HEAD_FLAG_BE			1
+#define HEAD_FLAG_PAGE_SIZE_4K		1
+#define HEAD_FLAG_PAGE_SIZE_16K		2
+#define HEAD_FLAG_PAGE_SIZE_64K		3
+
+#define head_flag_field(flags, field) \
+		(((flags) >> field##_SHIFT) & field##_MASK)
+
 /*
  * arm64 requires the DTB to be 8 byte aligned and
  * not exceed 2MB in size.
diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index b0853069702f..8cbac6232ed1 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -91,7 +91,7 @@ _head:
 	.quad	0				// reserved
 	.quad	0				// reserved
 	.quad	0				// reserved
-	.ascii	"ARM\x64"			// Magic number
+	.ascii	ARM64_MAGIC			// Magic number
 #ifdef CONFIG_EFI
 	.long	pe_header - _head		// Offset to the PE header.
 
-- 
2.18.0

[PATCH v14 08/16] arm64: cpufeature: add MMFR0 helper functions

[AKASHI Takahiro]

Those helper functions for MMFR0 register will be used later by kexec_file
loader.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Reviewed-by: James Morse <james.morse@arm.com>
---
 arch/arm64/include/asm/cpufeature.h | 48 +++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index 1717ba1db35d..cd90b5252d6d 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -486,11 +486,59 @@ static inline bool system_supports_32bit_el0(void)
 	return cpus_have_const_cap(ARM64_HAS_32BIT_EL0);
 }
 
+static inline bool system_supports_4kb_granule(void)
+{
+	u64 mmfr0;
+	u32 val;
+
+	mmfr0 =	read_sanitised_ftr_reg(SYS_ID_AA64MMFR0_EL1);
+	val = cpuid_feature_extract_unsigned_field(mmfr0,
+						ID_AA64MMFR0_TGRAN4_SHIFT);
+
+	return val == ID_AA64MMFR0_TGRAN4_SUPPORTED;
+}
+
+static inline bool system_supports_64kb_granule(void)
+{
+	u64 mmfr0;
+	u32 val;
+
+	mmfr0 =	read_sanitised_ftr_reg(SYS_ID_AA64MMFR0_EL1);
+	val = cpuid_feature_extract_unsigned_field(mmfr0,
+						ID_AA64MMFR0_TGRAN64_SHIFT);
+
+	return val == ID_AA64MMFR0_TGRAN64_SUPPORTED;
+}
+
+static inline bool system_supports_16kb_granule(void)
+{
+	u64 mmfr0;
+	u32 val;
+
+	mmfr0 =	read_sanitised_ftr_reg(SYS_ID_AA64MMFR0_EL1);
+	val = cpuid_feature_extract_unsigned_field(mmfr0,
+						ID_AA64MMFR0_TGRAN16_SHIFT);
+
+	return val == ID_AA64MMFR0_TGRAN16_SUPPORTED;
+}
+
 static inline bool system_supports_mixed_endian_el0(void)
 {
 	return id_aa64mmfr0_mixed_endian_el0(read_sanitised_ftr_reg(SYS_ID_AA64MMFR0_EL1));
 }
 
+static inline bool system_supports_mixed_endian(void)
+{
+	u64 mmfr0;
+	u32 val;
+
+	mmfr0 =	read_sanitised_ftr_reg(SYS_ID_AA64MMFR0_EL1);
+	val = cpuid_feature_extract_unsigned_field(mmfr0,
+						ID_AA64MMFR0_BIGENDEL_SHIFT);
+
+	return val == 0x1;
+}
+
 static inline bool system_supports_fpsimd(void)
 {
 	return !cpus_have_const_cap(ARM64_HAS_NO_FPSIMD);
-- 
2.18.0

[PATCH v14 09/16] arm64: enable KEXEC_FILE config

[AKASHI Takahiro]

Modify arm64/Kconfig to enable kexec_file_load support.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Acked-by: James Morse <james.morse@arm.com>
---
 arch/arm64/Kconfig                     |  9 +++++++++
 arch/arm64/kernel/Makefile             |  3 ++-
 arch/arm64/kernel/machine_kexec_file.c | 16 ++++++++++++++++
 3 files changed, 27 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm64/kernel/machine_kexec_file.c

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 1b1a0e95c751..5eb18d0cb513 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -843,6 +843,15 @@ config KEXEC
 	  but it is independent of the system firmware.   And like a reboot
 	  you can start any kernel with it, not just Linux.
 
+config KEXEC_FILE
+	bool "kexec file based system call"
+	select KEXEC_CORE
+	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.
+
 config CRASH_DUMP
 	bool "Build kdump crash kernel"
 	help
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index 95ac7374d723..8f1326b2d327 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -49,8 +49,9 @@ arm64-obj-$(CONFIG_ARM64_ACPI_PARKING_PROTOCOL)	+= acpi_parking_protocol.o
 arm64-obj-$(CONFIG_PARAVIRT)		+= paravirt.o
 arm64-obj-$(CONFIG_RANDOMIZE_BASE)	+= kaslr.o
 arm64-obj-$(CONFIG_HIBERNATION)		+= hibernate.o hibernate-asm.o
-arm64-obj-$(CONFIG_KEXEC)		+= machine_kexec.o relocate_kernel.o	\
+arm64-obj-$(CONFIG_KEXEC_CORE)		+= machine_kexec.o relocate_kernel.o	\
 					   cpu-reset.o
+arm64-obj-$(CONFIG_KEXEC_FILE)		+= machine_kexec_file.o
 arm64-obj-$(CONFIG_ARM64_RELOC_TEST)	+= arm64-reloc-test.o
 arm64-reloc-test-y := reloc_test_core.o reloc_test_syms.o
 arm64-obj-$(CONFIG_CRASH_DUMP)		+= crash_dump.o
diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c
new file mode 100644
index 000000000000..c38a8048ed00
--- /dev/null
+++ b/arch/arm64/kernel/machine_kexec_file.c
@@ -0,0 +1,16 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * kexec_file for arm64
+ *
+ * Copyright (C) 2018 Linaro Limited
+ * Author: AKASHI Takahiro <takahiro.akashi@linaro.org>
+ *
+ */
+
+#define pr_fmt(fmt) "kexec_file: " fmt
+
+#include <linux/kexec.h>
+
+const struct kexec_file_ops * const kexec_file_loaders[] = {
+	NULL
+};
-- 
2.18.0

[PATCH v14 10/16] arm64: kexec_file: load initrd and device-tree

[AKASHI Takahiro]

load_other_segments() is expected to allocate and place all the necessary
memory segments other than kernel, including initrd and device-tree
blob (and elf core header for crash).
While most of the code was borrowed from kexec-tools' counterpart,
users may not be allowed to specify dtb explicitly, instead, the dtb
presented by the original boot loader is reused.

arch_kimage_kernel_post_load_cleanup() is responsible for freeing arm64-
specific data allocated in load_other_segments().

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Reviewed-by: James Morse <james.morse@arm.com>
---
 arch/arm64/include/asm/kexec.h         |  17 +++
 arch/arm64/kernel/machine_kexec_file.c | 188 +++++++++++++++++++++++++
 2 files changed, 205 insertions(+)

diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
index e17f0529a882..157b2897d911 100644
--- a/arch/arm64/include/asm/kexec.h
+++ b/arch/arm64/include/asm/kexec.h
@@ -93,6 +93,23 @@ static inline void crash_prepare_suspend(void) {}
 static inline void crash_post_resume(void) {}
 #endif
 
+#ifdef CONFIG_KEXEC_FILE
+#define ARCH_HAS_KIMAGE_ARCH
+
+struct kimage_arch {
+	void *dtb;
+	unsigned long dtb_mem;
+};
+
+struct kimage;
+
+extern int arch_kimage_file_post_load_cleanup(struct kimage *image);
+extern int load_other_segments(struct kimage *image,
+		unsigned long kernel_load_addr, unsigned long kernel_size,
+		char *initrd, unsigned long initrd_len,
+		char *cmdline, unsigned long cmdline_len);
+#endif
+
 #endif /* __ASSEMBLY__ */
 
 #endif
diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c
index c38a8048ed00..f53f14bd1700 100644
--- a/arch/arm64/kernel/machine_kexec_file.c
+++ b/arch/arm64/kernel/machine_kexec_file.c
@@ -5,12 +5,200 @@
  * Copyright (C) 2018 Linaro Limited
  * Author: AKASHI Takahiro <takahiro.akashi@linaro.org>
  *
+ * Most code is derived from arm64 port of kexec-tools
  */
 
 #define pr_fmt(fmt) "kexec_file: " fmt
 
+#include <linux/ioport.h>
+#include <linux/kernel.h>
 #include <linux/kexec.h>
+#include <linux/libfdt.h>
+#include <linux/memblock.h>
+#include <linux/of_fdt.h>
+#include <linux/types.h>
+#include <asm/byteorder.h>
+
+/* relevant device tree properties */
+#define FDT_PSTR_INITRD_STA	"linux,initrd-start"
+#define FDT_PSTR_INITRD_END	"linux,initrd-end"
+#define FDT_PSTR_BOOTARGS	"bootargs"
 
 const struct kexec_file_ops * const kexec_file_loaders[] = {
 	NULL
 };
+
+int arch_kimage_file_post_load_cleanup(struct kimage *image)
+{
+	vfree(image->arch.dtb);
+	image->arch.dtb = NULL;
+
+	return kexec_image_post_load_cleanup_default(image);
+}
+
+static int setup_dtb(struct kimage *image,
+		unsigned long initrd_load_addr, unsigned long initrd_len,
+		char *cmdline, unsigned long cmdline_len,
+		void **dtb_buf, unsigned long *dtb_buf_len)
+{
+	void *buf = NULL;
+	size_t buf_size;
+	int nodeoffset;
+	int ret;
+
+	/* duplicate dt blob */
+	buf_size = fdt_totalsize(initial_boot_params);
+
+	if (initrd_load_addr) {
+		/* can be redundant, but trimmed at the end */
+		buf_size += fdt_prop_len(FDT_PSTR_INITRD_STA, sizeof(u64));
+		buf_size += fdt_prop_len(FDT_PSTR_INITRD_END, sizeof(u64));
+	}
+
+	if (cmdline)
+		/* can be redundant, but trimmed at the end */
+		buf_size += fdt_prop_len(FDT_PSTR_BOOTARGS, cmdline_len);
+
+	buf = vmalloc(buf_size);
+	if (!buf) {
+		ret = -ENOMEM;
+		goto out_err;
+	}
+
+	ret = fdt_open_into(initial_boot_params, buf, buf_size);
+	if (ret) {
+		ret = -EINVAL;
+		goto out_err;
+	}
+
+	nodeoffset = fdt_path_offset(buf, "/chosen");
+	if (nodeoffset < 0) {
+		ret = -EINVAL;
+		goto out_err;
+	}
+
+	/* add bootargs */
+	if (cmdline) {
+		ret = fdt_setprop_string(buf, nodeoffset, FDT_PSTR_BOOTARGS,
+							cmdline);
+		if (ret) {
+			ret = -EINVAL;
+			goto out_err;
+		}
+	} else {
+		ret = fdt_delprop(buf, nodeoffset, FDT_PSTR_BOOTARGS);
+		if (ret && (ret != -FDT_ERR_NOTFOUND)) {
+			ret = -EINVAL;
+			goto out_err;
+		}
+	}
+
+	/* add initrd-* */
+	if (initrd_load_addr) {
+		ret = fdt_setprop_u64(buf, nodeoffset, FDT_PSTR_INITRD_STA,
+							initrd_load_addr);
+		if (ret) {
+			ret = -EINVAL;
+			goto out_err;
+		}
+
+		ret = fdt_setprop_u64(buf, nodeoffset, FDT_PSTR_INITRD_END,
+						initrd_load_addr + initrd_len);
+		if (ret) {
+			ret = -EINVAL;
+			goto out_err;
+		}
+	} else {
+		ret = fdt_delprop(buf, nodeoffset, FDT_PSTR_INITRD_STA);
+		if (ret && (ret != -FDT_ERR_NOTFOUND)) {
+			ret = -EINVAL;
+			goto out_err;
+		}
+
+		ret = fdt_delprop(buf, nodeoffset, FDT_PSTR_INITRD_END);
+		if (ret && (ret != -FDT_ERR_NOTFOUND)) {
+			ret = -EINVAL;
+			goto out_err;
+		}
+	}
+
+	/* trim a buffer */
+	fdt_pack(buf);
+	*dtb_buf = buf;
+	*dtb_buf_len = fdt_totalsize(buf);
+
+	return 0;
+
+out_err:
+	vfree(buf);
+	return ret;
+}
+
+int load_other_segments(struct kimage *image,
+			unsigned long kernel_load_addr,
+			unsigned long kernel_size,
+			char *initrd, unsigned long initrd_len,
+			char *cmdline, unsigned long cmdline_len)
+{
+	struct kexec_buf kbuf;
+	void *dtb = NULL;
+	unsigned long initrd_load_addr = 0, dtb_len;
+	int ret = 0;
+
+	kbuf.image = image;
+	/* not allocate anything below the kernel */
+	kbuf.buf_min = kernel_load_addr + kernel_size;
+
+	/* load initrd */
+	if (initrd) {
+		kbuf.buffer = initrd;
+		kbuf.bufsz = initrd_len;
+		kbuf.mem = 0;
+		kbuf.memsz = initrd_len;
+		kbuf.buf_align = 0;
+		/* within 1GB-aligned window of up to 32GB in size */
+		kbuf.buf_max = round_down(kernel_load_addr, SZ_1G)
+						+ (unsigned long)SZ_1G * 32;
+		kbuf.top_down = false;
+
+		ret = kexec_add_buffer(&kbuf);
+		if (ret)
+			goto out_err;
+		initrd_load_addr = kbuf.mem;
+
+		pr_debug("Loaded initrd at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
+				initrd_load_addr, initrd_len, initrd_len);
+	}
+
+	/* load dtb blob */
+	ret = setup_dtb(image, initrd_load_addr, initrd_len,
+				cmdline, cmdline_len, &dtb, &dtb_len);
+	if (ret) {
+		pr_err("Preparing for new dtb failed\n");
+		goto out_err;
+	}
+
+	kbuf.buffer = dtb;
+	kbuf.bufsz = dtb_len;
+	kbuf.mem = 0;
+	kbuf.memsz = dtb_len;
+	/* not across 2MB boundary */
+	kbuf.buf_align = SZ_2M;
+	kbuf.buf_max = ULONG_MAX;
+	kbuf.top_down = true;
+
+	ret = kexec_add_buffer(&kbuf);
+	if (ret)
+		goto out_err;
+	image->arch.dtb = dtb;
+	image->arch.dtb_mem = kbuf.mem;
+
+	pr_debug("Loaded dtb at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
+			kbuf.mem, dtb_len, dtb_len);
+
+	return 0;
+
+out_err:
+	vfree(dtb);
+	return ret;
+}
-- 
2.18.0

[PATCH v14 11/16] arm64: kexec_file: allow for loading Image-format kernel

[AKASHI Takahiro]

This patch provides kexec_file_ops for "Image"-format kernel. In this
implementation, a binary is always loaded with a fixed offset identified
in text_offset field of its header.

Regarding signature verification for trusted boot, this patch doesn't
contains CONFIG_KEXEC_VERIFY_SIG support, which is to be added later
in this series, but file-attribute-based verification is still a viable
option by enabling IMA security subsystem.

You can sign(label) a to-be-kexec'ed kernel image on target file system
with:
    $ evmctl ima_sign --key /path/to/private_key.pem Image

On live system, you must have IMA enforced with, at least, the following
security policy:
    "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig"

See more details about IMA here:
    https://sourceforge.net/p/linux-ima/wiki/Home/

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Reviewed-by: James Morse <james.morse@arm.com>
---
 arch/arm64/include/asm/kexec.h         |  28 +++++++
 arch/arm64/kernel/Makefile             |   2 +-
 arch/arm64/kernel/kexec_image.c        | 108 +++++++++++++++++++++++++
 arch/arm64/kernel/machine_kexec_file.c |   1 +
 4 files changed, 138 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm64/kernel/kexec_image.c

diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
index 157b2897d911..5e673481b3a3 100644
--- a/arch/arm64/include/asm/kexec.h
+++ b/arch/arm64/include/asm/kexec.h
@@ -101,6 +101,34 @@ struct kimage_arch {
 	unsigned long dtb_mem;
 };
 
+/**
+ * struct arm64_image_header - arm64 kernel image header
+ * See Documentation/arm64/booting.txt for details
+ *
+ * @mz_magic: DOS header magic number ('MZ', optional)
+ * @code1: Instruction (branch to stext)
+ * @text_offset: Image load offset
+ * @image_size: Effective image size
+ * @flags: Bit-field flags
+ * @reserved: Reserved
+ * @magic: Magic number
+ * @pe_header: Offset to PE COFF header (optional)
+ **/
+
+struct arm64_image_header {
+	__le16 mz_magic; /* also code0 */
+	__le16 pad;
+	__le32 code1;
+	__le64 text_offset;
+	__le64 image_size;
+	__le64 flags;
+	__le64 reserved[3];
+	__le32 magic;
+	__le32 pe_header;
+};
+
+extern const struct kexec_file_ops kexec_image_ops;
+
 struct kimage;
 
 extern int arch_kimage_file_post_load_cleanup(struct kimage *image);
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index 8f1326b2d327..8cd514855eec 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -51,7 +51,7 @@ arm64-obj-$(CONFIG_RANDOMIZE_BASE)	+= kaslr.o
 arm64-obj-$(CONFIG_HIBERNATION)		+= hibernate.o hibernate-asm.o
 arm64-obj-$(CONFIG_KEXEC_CORE)		+= machine_kexec.o relocate_kernel.o	\
 					   cpu-reset.o
-arm64-obj-$(CONFIG_KEXEC_FILE)		+= machine_kexec_file.o
+arm64-obj-$(CONFIG_KEXEC_FILE)		+= machine_kexec_file.o kexec_image.o
 arm64-obj-$(CONFIG_ARM64_RELOC_TEST)	+= arm64-reloc-test.o
 arm64-reloc-test-y := reloc_test_core.o reloc_test_syms.o
 arm64-obj-$(CONFIG_CRASH_DUMP)		+= crash_dump.o
diff --git a/arch/arm64/kernel/kexec_image.c b/arch/arm64/kernel/kexec_image.c
new file mode 100644
index 000000000000..d64f5e9f9d22
--- /dev/null
+++ b/arch/arm64/kernel/kexec_image.c
@@ -0,0 +1,108 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Kexec image loader
+
+ * Copyright (C) 2018 Linaro Limited
+ * Author: AKASHI Takahiro <takahiro.akashi@linaro.org>
+ */
+
+#define pr_fmt(fmt)	"kexec_file(Image): " fmt
+
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/kexec.h>
+#include <linux/string.h>
+#include <asm/boot.h>
+#include <asm/byteorder.h>
+#include <asm/cpufeature.h>
+#include <asm/memory.h>
+
+static int image_probe(const char *kernel_buf, unsigned long kernel_len)
+{
+	const struct arm64_image_header *h;
+
+	h = (const struct arm64_image_header *)(kernel_buf);
+
+	if (!h || (kernel_len < sizeof(*h)) ||
+			memcmp(&h->magic, ARM64_MAGIC, sizeof(h->magic)))
+		return -EINVAL;
+
+	return 0;
+}
+
+static void *image_load(struct kimage *image,
+				char *kernel, unsigned long kernel_len,
+				char *initrd, unsigned long initrd_len,
+				char *cmdline, unsigned long cmdline_len)
+{
+	struct arm64_image_header *h;
+	u64 flags, value;
+	struct kexec_buf kbuf;
+	unsigned long text_offset;
+	struct kexec_segment *kernel_segment;
+	int ret;
+
+	/* Don't support old kernel */
+	h = (struct arm64_image_header *)kernel;
+	if (!h->text_offset)
+		return ERR_PTR(-EINVAL);
+
+	/* Check cpu features */
+	flags = le64_to_cpu(h->flags);
+	value = head_flag_field(flags, HEAD_FLAG_BE);
+	if (((value == HEAD_FLAG_BE) && !IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) ||
+	    ((value != HEAD_FLAG_BE) && IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)))
+		if (!system_supports_mixed_endian())
+			return ERR_PTR(-EINVAL);
+
+	value = head_flag_field(flags, HEAD_FLAG_PAGE_SIZE);
+	if (((value == HEAD_FLAG_PAGE_SIZE_4K) &&
+			!system_supports_4kb_granule()) ||
+	    ((value == HEAD_FLAG_PAGE_SIZE_64K) &&
+			!system_supports_64kb_granule()) ||
+	    ((value == HEAD_FLAG_PAGE_SIZE_16K) &&
+			!system_supports_16kb_granule()))
+		return ERR_PTR(-EINVAL);
+
+	/* Load the kernel */
+	kbuf.image = image;
+	kbuf.buf_min = 0;
+	kbuf.buf_max = ULONG_MAX;
+	kbuf.top_down = false;
+
+	kbuf.buffer = kernel;
+	kbuf.bufsz = kernel_len;
+	kbuf.mem = 0;
+	kbuf.memsz = le64_to_cpu(h->image_size);
+	text_offset = le64_to_cpu(h->text_offset);
+	kbuf.buf_align = MIN_KIMG_ALIGN;
+
+	/* Adjust kernel segment with TEXT_OFFSET */
+	kbuf.memsz += text_offset;
+
+	ret = kexec_add_buffer(&kbuf);
+	if (ret)
+		return ERR_PTR(ret);
+
+	kernel_segment = &image->segment[image->nr_segments - 1];
+	kernel_segment->mem += text_offset;
+	kernel_segment->memsz -= text_offset;
+	image->start = kernel_segment->mem;
+
+	pr_debug("Loaded kernel at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
+				kernel_segment->mem, kbuf.bufsz,
+				kernel_segment->memsz);
+
+	/* Load additional data */
+	ret = load_other_segments(image,
+				kernel_segment->mem, kernel_segment->memsz,
+				initrd, initrd_len, cmdline, cmdline_len);
+
+	return ERR_PTR(ret);
+}
+
+const struct kexec_file_ops kexec_image_ops = {
+	.probe = image_probe,
+	.load = image_load,
+};
diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c
index f53f14bd1700..05fb2d4e6fef 100644
--- a/arch/arm64/kernel/machine_kexec_file.c
+++ b/arch/arm64/kernel/machine_kexec_file.c
@@ -25,6 +25,7 @@
 #define FDT_PSTR_BOOTARGS	"bootargs"
 
 const struct kexec_file_ops * const kexec_file_loaders[] = {
+	&kexec_image_ops,
 	NULL
 };
 
-- 
2.18.0

[PATCH v14 12/16] arm64: kexec_file: add crash dump support

[AKASHI Takahiro]

Enabling crash dump (kdump) includes
* prepare contents of ELF header of a core dump file, /proc/vmcore,
  using crash_prepare_elf64_headers(), and
* add two device tree properties, "linux,usable-memory-range" and
  "linux,elfcorehdr", which represent respectively a memory range
  to be used by crash dump kernel and the header's location

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Reviewed-by: James Morse <james.morse@arm.com>
---
 arch/arm64/include/asm/kexec.h         |   4 +
 arch/arm64/kernel/machine_kexec_file.c | 113 ++++++++++++++++++++++++-
 2 files changed, 114 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
index 5e673481b3a3..1b2c27026ae0 100644
--- a/arch/arm64/include/asm/kexec.h
+++ b/arch/arm64/include/asm/kexec.h
@@ -99,6 +99,10 @@ static inline void crash_post_resume(void) {}
 struct kimage_arch {
 	void *dtb;
 	unsigned long dtb_mem;
+	/* Core ELF header buffer */
+	void *elf_headers;
+	unsigned long elf_headers_mem;
+	unsigned long elf_headers_sz;
 };
 
 /**
diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c
index 05fb2d4e6fef..ecaecb122cad 100644
--- a/arch/arm64/kernel/machine_kexec_file.c
+++ b/arch/arm64/kernel/machine_kexec_file.c
@@ -16,10 +16,14 @@
 #include <linux/libfdt.h>
 #include <linux/memblock.h>
 #include <linux/of_fdt.h>
+#include <linux/slab.h>
 #include <linux/types.h>
+#include <linux/vmalloc.h>
 #include <asm/byteorder.h>
 
 /* relevant device tree properties */
+#define FDT_PSTR_KEXEC_ELFHDR	"linux,elfcorehdr"
+#define FDT_PSTR_MEM_RANGE	"linux,usable-memory-range"
 #define FDT_PSTR_INITRD_STA	"linux,initrd-start"
 #define FDT_PSTR_INITRD_END	"linux,initrd-end"
 #define FDT_PSTR_BOOTARGS	"bootargs"
@@ -34,6 +38,10 @@ int arch_kimage_file_post_load_cleanup(struct kimage *image)
 	vfree(image->arch.dtb);
 	image->arch.dtb = NULL;
 
+	vfree(image->arch.elf_headers);
+	image->arch.elf_headers = NULL;
+	image->arch.elf_headers_sz = 0;
+
 	return kexec_image_post_load_cleanup_default(image);
 }
 
@@ -43,12 +51,29 @@ static int setup_dtb(struct kimage *image,
 		void **dtb_buf, unsigned long *dtb_buf_len)
 {
 	void *buf = NULL;
-	size_t buf_size;
+	size_t buf_size, range_size;
 	int nodeoffset;
 	int ret;
 
+	/* check ranges against root's #address-cells and #size-cells */
+	if (image->type == KEXEC_TYPE_CRASH &&
+		(!of_fdt_cells_size_fitted(image->arch.elf_headers_mem,
+				image->arch.elf_headers_sz) ||
+		 !of_fdt_cells_size_fitted(crashk_res.start,
+				crashk_res.end - crashk_res.start + 1))) {
+		pr_err("Crash memory region doesn't fit into DT's root cell sizes.\n");
+		ret = -EINVAL;
+		goto out_err;
+	}
+
 	/* duplicate dt blob */
 	buf_size = fdt_totalsize(initial_boot_params);
+	range_size = of_fdt_reg_cells_size();
+
+	if (image->type == KEXEC_TYPE_CRASH) {
+		buf_size += fdt_prop_len(FDT_PSTR_KEXEC_ELFHDR, range_size);
+		buf_size += fdt_prop_len(FDT_PSTR_MEM_RANGE, range_size);
+	}
 
 	if (initrd_load_addr) {
 		/* can be redundant, but trimmed at the end */
@@ -78,6 +103,22 @@ static int setup_dtb(struct kimage *image,
 		goto out_err;
 	}
 
+	if (image->type == KEXEC_TYPE_CRASH) {
+		/* add linux,elfcorehdr */
+		ret = fdt_setprop_reg(buf, nodeoffset, FDT_PSTR_KEXEC_ELFHDR,
+				image->arch.elf_headers_mem,
+				image->arch.elf_headers_sz);
+		if (ret)
+			goto out_err;
+
+		/* add linux,usable-memory-range */
+		ret = fdt_setprop_reg(buf, nodeoffset, FDT_PSTR_MEM_RANGE,
+				crashk_res.start,
+				crashk_res.end - crashk_res.start + 1);
+		if (ret)
+			goto out_err;
+	}
+
 	/* add bootargs */
 	if (cmdline) {
 		ret = fdt_setprop_string(buf, nodeoffset, FDT_PSTR_BOOTARGS,
@@ -135,6 +176,43 @@ static int setup_dtb(struct kimage *image,
 	return ret;
 }
 
+static int prepare_elf_headers(void **addr, unsigned long *sz)
+{
+	struct crash_mem *cmem;
+	unsigned int nr_ranges;
+	int ret;
+	u64 i;
+	phys_addr_t start, end;
+
+	nr_ranges = 1; /* for exclusion of crashkernel region */
+	for_each_mem_range(i, &memblock.memory, NULL, NUMA_NO_NODE,
+					MEMBLOCK_NONE, &start, &end, NULL)
+		nr_ranges++;
+
+	cmem = kmalloc(sizeof(struct crash_mem) +
+			sizeof(struct crash_mem_range) * nr_ranges, GFP_KERNEL);
+	if (!cmem)
+		return -ENOMEM;
+
+	cmem->max_nr_ranges = nr_ranges;
+	cmem->nr_ranges = 0;
+	for_each_mem_range(i, &memblock.memory, NULL, NUMA_NO_NODE,
+					MEMBLOCK_NONE, &start, &end, NULL) {
+		cmem->ranges[cmem->nr_ranges].start = start;
+		cmem->ranges[cmem->nr_ranges].end = end - 1;
+		cmem->nr_ranges++;
+	}
+
+	/* Exclude crashkernel region */
+	ret = crash_exclude_mem_range(cmem, crashk_res.start, crashk_res.end);
+
+	if (!ret)
+		ret =  crash_prepare_elf64_headers(cmem, true, addr, sz);
+
+	kfree(cmem);
+	return ret;
+}
+
 int load_other_segments(struct kimage *image,
 			unsigned long kernel_load_addr,
 			unsigned long kernel_size,
@@ -142,14 +220,43 @@ int load_other_segments(struct kimage *image,
 			char *cmdline, unsigned long cmdline_len)
 {
 	struct kexec_buf kbuf;
-	void *dtb = NULL;
-	unsigned long initrd_load_addr = 0, dtb_len;
+	void *headers, *dtb = NULL;
+	unsigned long headers_sz, initrd_load_addr = 0, dtb_len;
 	int ret = 0;
 
 	kbuf.image = image;
 	/* not allocate anything below the kernel */
 	kbuf.buf_min = kernel_load_addr + kernel_size;
 
+	/* load elf core header */
+	if (image->type == KEXEC_TYPE_CRASH) {
+		ret = prepare_elf_headers(&headers, &headers_sz);
+		if (ret) {
+			pr_err("Preparing elf core header failed\n");
+			goto out_err;
+		}
+
+		kbuf.buffer = headers;
+		kbuf.bufsz = headers_sz;
+		kbuf.mem = 0;
+		kbuf.memsz = headers_sz;
+		kbuf.buf_align = SZ_64K; /* largest supported page size */
+		kbuf.buf_max = ULONG_MAX;
+		kbuf.top_down = true;
+
+		ret = kexec_add_buffer(&kbuf);
+		if (ret) {
+			vfree(headers);
+			goto out_err;
+		}
+		image->arch.elf_headers = headers;
+		image->arch.elf_headers_mem = kbuf.mem;
+		image->arch.elf_headers_sz = headers_sz;
+
+		pr_debug("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
+			 image->arch.elf_headers_mem, headers_sz, headers_sz);
+	}
+
 	/* load initrd */
 	if (initrd) {
 		kbuf.buffer = initrd;
-- 
2.18.0

[PATCH v14 13/16] arm64: kexec_file: invoke the kernel without purgatory

[AKASHI Takahiro]

On arm64, purgatory would do almost nothing. So just invoke secondary
kernel directly by jumping into its entry code.

While, in this case, cpu_soft_restart() must be called with dtb address
in the fifth argument, the behavior still stays compatible with kexec_load
case as long as the argument is null.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reviewed-by: James Morse <james.morse@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/kernel/cpu-reset.S       |  8 ++++----
 arch/arm64/kernel/machine_kexec.c   | 12 ++++++++++--
 arch/arm64/kernel/relocate_kernel.S |  3 ++-
 3 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kernel/cpu-reset.S b/arch/arm64/kernel/cpu-reset.S
index 8021b46c9743..a2be30275a73 100644
--- a/arch/arm64/kernel/cpu-reset.S
+++ b/arch/arm64/kernel/cpu-reset.S
@@ -22,11 +22,11 @@
  * __cpu_soft_restart(el2_switch, entry, arg0, arg1, arg2) - Helper for
  * cpu_soft_restart.
  *
- * @el2_switch: Flag to indicate a swich to EL2 is needed.
+ * @el2_switch: Flag to indicate a switch to EL2 is needed.
  * @entry: Location to jump to for soft reset.
- * arg0: First argument passed to @entry.
- * arg1: Second argument passed to @entry.
- * arg2: Third argument passed to @entry.
+ * arg0: First argument passed to @entry. (relocation list)
+ * arg1: Second argument passed to @entry.(physical kernel entry)
+ * arg2: Third argument passed to @entry. (physical dtb address)
  *
  * Put the CPU into the same state as it would be if it had been reset, and
  * branch to what would be the reset vector. It must be executed with the
diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c
index f6a5c6bc1434..c63c355e2f18 100644
--- a/arch/arm64/kernel/machine_kexec.c
+++ b/arch/arm64/kernel/machine_kexec.c
@@ -212,9 +212,17 @@ void machine_kexec(struct kimage *kimage)
 	 * uses physical addressing to relocate the new image to its final
 	 * position and transfers control to the image entry point when the
 	 * relocation is complete.
+	 * In kexec case, kimage->start points to purgatory assuming that
+	 * kernel entry and dtb address are embedded in purgatory by
+	 * userspace (kexec-tools).
+	 * In kexec_file case, the kernel starts directly without purgatory.
 	 */
-
-	cpu_soft_restart(reboot_code_buffer_phys, kimage->head, kimage->start, 0);
+	cpu_soft_restart(reboot_code_buffer_phys, kimage->head, kimage->start,
+#ifdef CONFIG_KEXEC_FILE
+						kimage->arch.dtb_mem);
+#else
+						0);
+#endif
 
 	BUG(); /* Should never get here. */
 }
diff --git a/arch/arm64/kernel/relocate_kernel.S b/arch/arm64/kernel/relocate_kernel.S
index f407e422a720..95fd94209aae 100644
--- a/arch/arm64/kernel/relocate_kernel.S
+++ b/arch/arm64/kernel/relocate_kernel.S
@@ -32,6 +32,7 @@
 ENTRY(arm64_relocate_new_kernel)
 
 	/* Setup the list loop variables. */
+	mov	x18, x2				/* x18 = dtb address */
 	mov	x17, x1				/* x17 = kimage_start */
 	mov	x16, x0				/* x16 = kimage_head */
 	raw_dcache_line_size x15, x0		/* x15 = dcache line size */
@@ -107,7 +108,7 @@ ENTRY(arm64_relocate_new_kernel)
 	isb
 
 	/* Start new image. */
-	mov	x0, xzr
+	mov	x0, x18
 	mov	x1, xzr
 	mov	x2, xzr
 	mov	x3, xzr
-- 
2.18.0

[PATCH v14 14/16] include: pe.h: remove message[] from mz header definition

[AKASHI Takahiro]

message[] field won't be part of the definition of mz header.

This change is crucial for enabling kexec_file_load on arm64 because
arm64's "Image" binary, as in PE format, doesn't have any data for it and
accordingly the following check in pefile_parse_binary() will fail:

	chkaddr(cursor, mz->peaddr, sizeof(*pe));

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: David Howells <dhowells@redhat.com>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: David S. Miller <davem@davemloft.net>
---
 include/linux/pe.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/pe.h b/include/linux/pe.h
index 143ce75be5f0..3482b18a48b5 100644
--- a/include/linux/pe.h
+++ b/include/linux/pe.h
@@ -166,7 +166,7 @@ struct mz_hdr {
 	uint16_t oem_info;	/* oem specific */
 	uint16_t reserved1[10];	/* reserved */
 	uint32_t peaddr;	/* address of pe header */
-	char     message[64];	/* message to print */
+	char     message[];	/* message to print */
 };
 
 struct mz_reloc {
-- 
2.18.0

[PATCH v14 15/16] arm64: kexec_file: add kernel signature verification support

[AKASHI Takahiro]

With this patch, kernel verification can be done without IMA security
subsystem enabled. Turn on CONFIG_KEXEC_VERIFY_SIG instead.

On x86, a signature is embedded into a PE file (Microsoft's format) header
of binary. Since arm64's "Image" can also be seen as a PE file as far as
CONFIG_EFI is enabled, we adopt this format for kernel signing.

You can create a signed kernel image with:
    $ sbsign --key ${KEY} --cert ${CERT} Image

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Reviewed-by: James Morse <james.morse@arm.com>
---
 arch/arm64/Kconfig              | 24 ++++++++++++++++++++++++
 arch/arm64/kernel/kexec_image.c | 15 +++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 5eb18d0cb513..e7de9500bf0b 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -852,6 +852,30 @@ config KEXEC_FILE
 	  for kernel and initramfs as opposed to list of segments as
 	  accepted by previous system call.
 
+config KEXEC_VERIFY_SIG
+	bool "Verify kernel signature during kexec_file_load() syscall"
+	depends on KEXEC_FILE
+	help
+	  Select this option to verify a signature with loaded kernel
+	  image. If configured, any attempt of loading a image without
+	  valid signature will fail.
+
+	  In addition to that option, you need to enable signature
+	  verification for the corresponding kernel image type being
+	  loaded in order for this to work.
+
+config KEXEC_IMAGE_VERIFY_SIG
+	bool "Enable Image signature verification support"
+	default y
+	depends on KEXEC_VERIFY_SIG
+	depends on EFI && SIGNED_PE_FILE_VERIFICATION
+	help
+	  Enable Image signature verification support.
+
+comment "Support for PE file signature verification disabled"
+	depends on KEXEC_VERIFY_SIG
+	depends on !EFI || !SIGNED_PE_FILE_VERIFICATION
+
 config CRASH_DUMP
 	bool "Build kdump crash kernel"
 	help
diff --git a/arch/arm64/kernel/kexec_image.c b/arch/arm64/kernel/kexec_image.c
index d64f5e9f9d22..578d358632d0 100644
--- a/arch/arm64/kernel/kexec_image.c
+++ b/arch/arm64/kernel/kexec_image.c
@@ -13,6 +13,7 @@
 #include <linux/kernel.h>
 #include <linux/kexec.h>
 #include <linux/string.h>
+#include <linux/verification.h>
 #include <asm/boot.h>
 #include <asm/byteorder.h>
 #include <asm/cpufeature.h>
@@ -28,6 +29,9 @@ static int image_probe(const char *kernel_buf, unsigned long kernel_len)
 			memcmp(&h->magic, ARM64_MAGIC, sizeof(h->magic)))
 		return -EINVAL;
 
+	pr_debug("PE format: %s\n",
+			memcmp(&h->mz_magic, "MZ", 2) ?  "no" : "yes");
+
 	return 0;
 }
 
@@ -102,7 +106,18 @@ static void *image_load(struct kimage *image,
 	return ERR_PTR(ret);
 }
 
+#ifdef CONFIG_KEXEC_IMAGE_VERIFY_SIG
+static int image_verify_sig(const char *kernel, unsigned long kernel_len)
+{
+	return verify_pefile_signature(kernel, kernel_len, NULL,
+				       VERIFYING_KEXEC_PE_SIGNATURE);
+}
+#endif
+
 const struct kexec_file_ops kexec_image_ops = {
 	.probe = image_probe,
 	.load = image_load,
+#ifdef CONFIG_KEXEC_IMAGE_VERIFY_SIG
+	.verify_sig = image_verify_sig,
+#endif
 };
-- 
2.18.0

[PATCH v14 16/16] arm64: kexec_file: add kaslr support

[AKASHI Takahiro]

Adding "kaslr-seed" to dtb enables triggering kaslr, or kernel virtual
address randomization, at secondary kernel boot. We always do this as
it will have no harm on kaslr-incapable kernel.

We don't have any "switch" to turn off this feature directly, but still
can suppress it by passing "nokaslr" as a kernel boot argument.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/kernel/machine_kexec_file.c | 45 ++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c
index ecaecb122cad..967db9824e3f 100644
--- a/arch/arm64/kernel/machine_kexec_file.c
+++ b/arch/arm64/kernel/machine_kexec_file.c
@@ -16,6 +16,7 @@
 #include <linux/libfdt.h>
 #include <linux/memblock.h>
 #include <linux/of_fdt.h>
+#include <linux/random.h>
 #include <linux/slab.h>
 #include <linux/types.h>
 #include <linux/vmalloc.h>
@@ -27,6 +28,7 @@
 #define FDT_PSTR_INITRD_STA	"linux,initrd-start"
 #define FDT_PSTR_INITRD_END	"linux,initrd-end"
 #define FDT_PSTR_BOOTARGS	"bootargs"
+#define FDT_PSTR_KASLR_SEED	"kaslr-seed"
 
 const struct kexec_file_ops * const kexec_file_loaders[] = {
 	&kexec_image_ops,
@@ -45,6 +47,32 @@ int arch_kimage_file_post_load_cleanup(struct kimage *image)
 	return kexec_image_post_load_cleanup_default(image);
 }
 
+/* crng needs to have been initialized for providing kaslr-seed */
+static int random_ready;
+
+static void random_ready_notified(struct random_ready_callback *unused)
+{
+	random_ready = 1;
+}
+
+static struct random_ready_callback random_ready_cb = {
+	.func = random_ready_notified,
+};
+
+static __init int init_random_ready_cb(void)
+{
+	int ret;
+
+	ret = add_random_ready_callback(&random_ready_cb);
+	if (ret == -EALREADY)
+		random_ready = 1;
+	else if (ret)
+		pr_warn("failed to add a callback for random_ready\n");
+
+	return 0;
+}
+late_initcall(init_random_ready_cb)
+
 static int setup_dtb(struct kimage *image,
 		unsigned long initrd_load_addr, unsigned long initrd_len,
 		char *cmdline, unsigned long cmdline_len,
@@ -53,6 +81,7 @@ static int setup_dtb(struct kimage *image,
 	void *buf = NULL;
 	size_t buf_size, range_size;
 	int nodeoffset;
+	u64 value;
 	int ret;
 
 	/* check ranges against root's #address-cells and #size-cells */
@@ -85,6 +114,8 @@ static int setup_dtb(struct kimage *image,
 		/* can be redundant, but trimmed at the end */
 		buf_size += fdt_prop_len(FDT_PSTR_BOOTARGS, cmdline_len);
 
+	buf_size += fdt_prop_len(FDT_PSTR_KASLR_SEED, sizeof(u64));
+
 	buf = vmalloc(buf_size);
 	if (!buf) {
 		ret = -ENOMEM;
@@ -164,6 +195,20 @@ static int setup_dtb(struct kimage *image,
 		}
 	}
 
+	/* add kaslr-seed */
+	fdt_delprop(buf, nodeoffset, FDT_PSTR_KASLR_SEED);
+	if (random_ready) {
+		get_random_bytes(&value, sizeof(value));
+		ret = fdt_setprop_u64(buf, nodeoffset, FDT_PSTR_KASLR_SEED,
+							value);
+		if (ret) {
+			ret = -EINVAL;
+			goto out_err;
+		}
+	} else {
+		pr_notice("kaslr-seed won't be fed\n");
+	}
+
 	/* trim a buffer */
 	fdt_pack(buf);
 	*dtb_buf = buf;
-- 
2.18.0

Re: [PATCH v14 06/16] of/fdt: add helper functions for handling properties

[Frank Rowand]

On 09/07/18 01:00, AKASHI Takahiro wrote:
> These functions will be used later to handle kexec-specific properties
> in arm64's kexec_file implementation.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Frank Rowand <frowand.list@gmail.com>
> ---
>  drivers/of/fdt.c       | 62 ++++++++++++++++++++++++++++++++++++++++--
>  include/linux/of_fdt.h | 10 +++++--
>  2 files changed, 68 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 800ad252cf9c..dc960cea1355 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -25,6 +25,7 @@
>  #include <linux/debugfs.h>
>  #include <linux/serial_core.h>
>  #include <linux/sysfs.h>
> +#include <linux/types.h>
>  
>  #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
>  #include <asm/page.h>
> @@ -537,8 +538,8 @@ void *of_fdt_unflatten_tree(const unsigned long *blob,
>  EXPORT_SYMBOL_GPL(of_fdt_unflatten_tree);
>  
>  /* Everything below here references initial_boot_params directly. */
> -int __initdata dt_root_addr_cells;
> -int __initdata dt_root_size_cells;
> +int dt_root_addr_cells;
> +int dt_root_size_cells;
>  
>  void *initial_boot_params;
>  
> @@ -1323,3 +1324,60 @@ late_initcall(of_fdt_raw_init);
>  #endif
>  
>  #endif /* CONFIG_OF_EARLY_FLATTREE */
> +

Please add comment:

/* helper functions for arm64 kexec */


> +bool of_fdt_cells_size_fitted(u64 base, u64 size)

Please rename as of_fdt_range_valid()


> +{
> +	/* if *_cells >= 2, cells can hold 64-bit values anyway */
> +	if ((dt_root_addr_cells == 1) && (base > U32_MAX))
> +		return false;
> +
> +	if ((dt_root_size_cells == 1) && (size > U32_MAX))
> +		return false;

Should also check that base + size does not wrap around.


> +
> +	return true;
> +}
> +
> +size_t of_fdt_reg_cells_size(void)

Please rename as of_fdt_root_range_size()


> +{
> +	return (dt_root_addr_cells + dt_root_size_cells) * sizeof(u32);
> +}
> +
> +#define FDT_ALIGN(x, a)	(((x) + (a) - 1) & ~((a) - 1))
> +#define FDT_TAGALIGN(x)	(FDT_ALIGN((x), FDT_TAGSIZE))
> +
> +int fdt_prop_len(const char *prop_name, int len)

Please rename as fdt_len_added_prop()


> +{
> +	return (strlen(prop_name) + 1) +
> +		sizeof(struct fdt_property) +
> +		FDT_TAGALIGN(len);
> +}
> +

Please add comment, something like:

/* cells must be 1 or 2 */


> +static void fill_property(void *buf, u64 val64, int cells)

Please rename as cpu64_to_fdt_cells()

Thanks,

Frank

> +{
> +	__be32 val32;
> +
> +	while (cells) {
> +		val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
> +		memcpy(buf, &val32, sizeof(val32));
> +		buf += sizeof(val32);
> +	}
> +}
> +
> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
> +						u64 addr, u64 size)
> +{
> +	char buf[sizeof(__be32) * 2 * 2];
> +		/* assume dt_root_[addr|size]_cells <= 2 */
> +	void *prop;
> +	size_t buf_size;
> +
> +	buf_size = of_fdt_reg_cells_size();
> +	prop = buf;
> +
> +	fill_property(prop, addr, dt_root_addr_cells);
> +	prop += dt_root_addr_cells * sizeof(u32);
> +
> +	fill_property(prop, size, dt_root_size_cells);
> +
> +	return fdt_setprop(fdt, nodeoffset, name, buf, buf_size);
> +}
> diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
> index b9cd9ebdf9b9..9615d6142578 100644
> --- a/include/linux/of_fdt.h
> +++ b/include/linux/of_fdt.h
> @@ -37,8 +37,8 @@ extern void *of_fdt_unflatten_tree(const unsigned long *blob,
>  				   struct device_node **mynodes);
>  
>  /* TBD: Temporary export of fdt globals - remove when code fully merged */
> -extern int __initdata dt_root_addr_cells;
> -extern int __initdata dt_root_size_cells;
> +extern int dt_root_addr_cells;
> +extern int dt_root_size_cells;
>  extern void *initial_boot_params;
>  
>  extern char __dtb_start[];
> @@ -108,5 +108,11 @@ static inline void unflatten_device_tree(void) {}
>  static inline void unflatten_and_copy_device_tree(void) {}
>  #endif /* CONFIG_OF_EARLY_FLATTREE */
>  
> +bool of_fdt_cells_size_fitted(u64 base, u64 size);
> +size_t of_fdt_reg_cells_size(void);
> +int fdt_prop_len(const char *prop_name, int len);
> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
> +						u64 addr, u64 size);
> +
>  #endif /* __ASSEMBLY__ */
>  #endif /* _LINUX_OF_FDT_H */
> 

Re: [PATCH v14 06/16] of/fdt: add helper functions for handling properties

[AKASHI Takahiro]

Frank,

Thank you for the comments. I will address all of them, but
have one question:

On Fri, Sep 07, 2018 at 12:53:58PM -0700, Frank Rowand wrote:
> On 09/07/18 01:00, AKASHI Takahiro wrote:
> > These functions will be used later to handle kexec-specific properties
> > in arm64's kexec_file implementation.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Frank Rowand <frowand.list@gmail.com>
> > ---
> >  drivers/of/fdt.c       | 62 ++++++++++++++++++++++++++++++++++++++++--
> >  include/linux/of_fdt.h | 10 +++++--
> >  2 files changed, 68 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index 800ad252cf9c..dc960cea1355 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -25,6 +25,7 @@
> >  #include <linux/debugfs.h>
> >  #include <linux/serial_core.h>
> >  #include <linux/sysfs.h>
> > +#include <linux/types.h>
> >  
> >  #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
> >  #include <asm/page.h>
> > @@ -537,8 +538,8 @@ void *of_fdt_unflatten_tree(const unsigned long *blob,
> >  EXPORT_SYMBOL_GPL(of_fdt_unflatten_tree);
> >  
> >  /* Everything below here references initial_boot_params directly. */
> > -int __initdata dt_root_addr_cells;
> > -int __initdata dt_root_size_cells;
> > +int dt_root_addr_cells;
> > +int dt_root_size_cells;
> >  
> >  void *initial_boot_params;
> >  
> > @@ -1323,3 +1324,60 @@ late_initcall(of_fdt_raw_init);
> >  #endif
> >  
> >  #endif /* CONFIG_OF_EARLY_FLATTREE */
> > +
> 
> Please add comment:
> 
> /* helper functions for arm64 kexec */
> 
> 
> > +bool of_fdt_cells_size_fitted(u64 base, u64 size)
> 
> Please rename as of_fdt_range_valid()
> 
> 
> > +{
> > +	/* if *_cells >= 2, cells can hold 64-bit values anyway */
> > +	if ((dt_root_addr_cells == 1) && (base > U32_MAX))
> > +		return false;
> > +
> > +	if ((dt_root_size_cells == 1) && (size > U32_MAX))
> > +		return false;
> 
> Should also check that base + size does not wrap around.

What is the upper limit here?
For instance, #address_cells = <1> and #size_cells = <1>,
and can 'base + size' be over U32_MAX?
Assuming 'not' is quite reasonable, but it seems to me
that devicetree spec doesn't exclude it, as least I couldn't
find any notes about such a case.
(In my understands, #address_cells only restricts a size in 'reg' property.)

Thanks,
-Takahiro Akashi

> 
> > +
> > +	return true;
> > +}
> > +
> > +size_t of_fdt_reg_cells_size(void)
> 
> Please rename as of_fdt_root_range_size()
> 
> 
> > +{
> > +	return (dt_root_addr_cells + dt_root_size_cells) * sizeof(u32);
> > +}
> > +
> > +#define FDT_ALIGN(x, a)	(((x) + (a) - 1) & ~((a) - 1))
> > +#define FDT_TAGALIGN(x)	(FDT_ALIGN((x), FDT_TAGSIZE))
> > +
> > +int fdt_prop_len(const char *prop_name, int len)
> 
> Please rename as fdt_len_added_prop()
> 
> 
> > +{
> > +	return (strlen(prop_name) + 1) +
> > +		sizeof(struct fdt_property) +
> > +		FDT_TAGALIGN(len);
> > +}
> > +
> 
> Please add comment, something like:
> 
> /* cells must be 1 or 2 */
> 
> 
> > +static void fill_property(void *buf, u64 val64, int cells)
> 
> Please rename as cpu64_to_fdt_cells()
> 
> Thanks,
> 
> Frank
> 
> > +{
> > +	__be32 val32;
> > +
> > +	while (cells) {
> > +		val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
> > +		memcpy(buf, &val32, sizeof(val32));
> > +		buf += sizeof(val32);
> > +	}
> > +}
> > +
> > +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
> > +						u64 addr, u64 size)
> > +{
> > +	char buf[sizeof(__be32) * 2 * 2];
> > +		/* assume dt_root_[addr|size]_cells <= 2 */
> > +	void *prop;
> > +	size_t buf_size;
> > +
> > +	buf_size = of_fdt_reg_cells_size();
> > +	prop = buf;
> > +
> > +	fill_property(prop, addr, dt_root_addr_cells);
> > +	prop += dt_root_addr_cells * sizeof(u32);
> > +
> > +	fill_property(prop, size, dt_root_size_cells);
> > +
> > +	return fdt_setprop(fdt, nodeoffset, name, buf, buf_size);
> > +}
> > diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
> > index b9cd9ebdf9b9..9615d6142578 100644
> > --- a/include/linux/of_fdt.h
> > +++ b/include/linux/of_fdt.h
> > @@ -37,8 +37,8 @@ extern void *of_fdt_unflatten_tree(const unsigned long *blob,
> >  				   struct device_node **mynodes);
> >  
> >  /* TBD: Temporary export of fdt globals - remove when code fully merged */
> > -extern int __initdata dt_root_addr_cells;
> > -extern int __initdata dt_root_size_cells;
> > +extern int dt_root_addr_cells;
> > +extern int dt_root_size_cells;
> >  extern void *initial_boot_params;
> >  
> >  extern char __dtb_start[];
> > @@ -108,5 +108,11 @@ static inline void unflatten_device_tree(void) {}
> >  static inline void unflatten_and_copy_device_tree(void) {}
> >  #endif /* CONFIG_OF_EARLY_FLATTREE */
> >  
> > +bool of_fdt_cells_size_fitted(u64 base, u64 size);
> > +size_t of_fdt_reg_cells_size(void);
> > +int fdt_prop_len(const char *prop_name, int len);
> > +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
> > +						u64 addr, u64 size);
> > +
> >  #endif /* __ASSEMBLY__ */
> >  #endif /* _LINUX_OF_FDT_H */
> > 
> 

Re: [PATCH v14 06/16] of/fdt: add helper functions for handling properties

[Frank Rowand]

I was re-reading this while answering a later email in the thread.  After reading
other patches in the series that were not sent to me, I have a better understanding
of the intent behind this patch, and some changes to my previous reply.

The intent of the helper functions is related to properties whose values are
tuples of the same format as the "reg" property of the "/memory" nodes.  For
example, the "linux,usable-memory-range" and "linux,elfcoredhr" properties of
the "/chosen" node.

The patch header and the function names should be updated to reflect this intent.
This means most or all of my previous suggested function name changes are no longer
useful.

Please add devicetree@vger.kernel.org to the next version of this patch and to
the patches that use the functions in this patch.


On 09/07/18 12:53, Frank Rowand wrote:
> On 09/07/18 01:00, AKASHI Takahiro wrote:
>> These functions will be used later to handle kexec-specific properties
>> in arm64's kexec_file implementation.
>>
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Frank Rowand <frowand.list@gmail.com>
>> ---
>>  drivers/of/fdt.c       | 62 ++++++++++++++++++++++++++++++++++++++++--
>>  include/linux/of_fdt.h | 10 +++++--
>>  2 files changed, 68 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>> index 800ad252cf9c..dc960cea1355 100644
>> --- a/drivers/of/fdt.c
>> +++ b/drivers/of/fdt.c
>> @@ -25,6 +25,7 @@
>>  #include <linux/debugfs.h>
>>  #include <linux/serial_core.h>
>>  #include <linux/sysfs.h>
>> +#include <linux/types.h>
>>  
>>  #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
>>  #include <asm/page.h>
>> @@ -537,8 +538,8 @@ void *of_fdt_unflatten_tree(const unsigned long *blob,
>>  EXPORT_SYMBOL_GPL(of_fdt_unflatten_tree);
>>  
>>  /* Everything below here references initial_boot_params directly. */
>> -int __initdata dt_root_addr_cells;
>> -int __initdata dt_root_size_cells;
>> +int dt_root_addr_cells;
>> +int dt_root_size_cells;
>>  
>>  void *initial_boot_params;
>>  
>> @@ -1323,3 +1324,60 @@ late_initcall(of_fdt_raw_init);
>>  #endif
>>  
>>  #endif /* CONFIG_OF_EARLY_FLATTREE */
>> +

Global comment: this code should not be using the variables
dt_root_addr_cells and dt_root_size_cells.  These variables are
__initdata.

The code that is using these helpers is acting upon a specific FDT
(copied from initial_boot_params).  This code should be getting the
values of the root node's "#address-cells" and "#size-cells" from
the FDT.


> Please add comment:
> 
> /* helper functions for arm64 kexec */
> 
> 
>> +bool of_fdt_cells_size_fitted(u64 base, u64 size)
> 
> Please rename as of_fdt_range_valid()

I'm not entirely sure of what the caller in 12/16 is trying to ensure
with this function.

(1) At the minimum (and what the implementation in of_fdt_cells_size_fitted()
does) is make sure that an address and size tuple are consistent with
the root properties "#address-cells" and "#size-cells".

The caller in 12/16 is using this check to validate values for the
properties  "linux,elfcorehdr" and "linux,usable-memory-range".

(2) A more complete check _might_ be to ensure that the values also
specify memory that is available to the kernel.  This memory is described
by the "reg" property of one or more "/memory" nodes.

This second check is probably what is actually desired.

One possible issue to note is that the binding for "linux,usable-memory-range"
suggests that available memory could be described by an EFI memory map.
I am not familiar with how or when an EFI memory map might exist instead
of the "/memory" nodes.


>> +{
>> +	/* if *_cells >= 2, cells can hold 64-bit values anyway */
>> +	if ((dt_root_addr_cells == 1) && (base > U32_MAX))
>> +		return false;
>> +
>> +	if ((dt_root_size_cells == 1) && (size > U32_MAX))
>> +		return false;
> 
> Should also check that base + size does not wrap around.
> 
> 
>> +
>> +	return true;
>> +}
>> +
>> +size_t of_fdt_reg_cells_size(void)
> 
> Please rename as of_fdt_root_range_size()

Even better would be to remove this function and replace the one place
that it is called from with the one line of code in this function.

-Frank


>> +{
>> +	return (dt_root_addr_cells + dt_root_size_cells) * sizeof(u32);
>> +}
>> +
>> +#define FDT_ALIGN(x, a)	(((x) + (a) - 1) & ~((a) - 1))
>> +#define FDT_TAGALIGN(x)	(FDT_ALIGN((x), FDT_TAGSIZE))
>> +
>> +int fdt_prop_len(const char *prop_name, int len)
> 
> Please rename as fdt_len_added_prop()
> 
> 
>> +{
>> +	return (strlen(prop_name) + 1) +
>> +		sizeof(struct fdt_property) +
>> +		FDT_TAGALIGN(len);
>> +}
>> +
> 
> Please add comment, something like:
> 
> /* cells must be 1 or 2 */
> 
> 
>> +static void fill_property(void *buf, u64 val64, int cells)
> 
> Please rename as cpu64_to_fdt_cells()
> 
> Thanks,
> 
> Frank
> 
>> +{
>> +	__be32 val32;
>> +
>> +	while (cells) {
>> +		val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
>> +		memcpy(buf, &val32, sizeof(val32));
>> +		buf += sizeof(val32);
>> +	}
>> +}
>> +
>> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
>> +						u64 addr, u64 size)
>> +{
>> +	char buf[sizeof(__be32) * 2 * 2];
>> +		/* assume dt_root_[addr|size]_cells <= 2 */
>> +	void *prop;
>> +	size_t buf_size;
>> +
>> +	buf_size = of_fdt_reg_cells_size();
>> +	prop = buf;
>> +
>> +	fill_property(prop, addr, dt_root_addr_cells);
>> +	prop += dt_root_addr_cells * sizeof(u32);
>> +
>> +	fill_property(prop, size, dt_root_size_cells);
>> +
>> +	return fdt_setprop(fdt, nodeoffset, name, buf, buf_size);
>> +}
>> diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
>> index b9cd9ebdf9b9..9615d6142578 100644
>> --- a/include/linux/of_fdt.h
>> +++ b/include/linux/of_fdt.h
>> @@ -37,8 +37,8 @@ extern void *of_fdt_unflatten_tree(const unsigned long *blob,
>>  				   struct device_node **mynodes);
>>  
>>  /* TBD: Temporary export of fdt globals - remove when code fully merged */
>> -extern int __initdata dt_root_addr_cells;
>> -extern int __initdata dt_root_size_cells;
>> +extern int dt_root_addr_cells;
>> +extern int dt_root_size_cells;
>>  extern void *initial_boot_params;
>>  
>>  extern char __dtb_start[];
>> @@ -108,5 +108,11 @@ static inline void unflatten_device_tree(void) {}
>>  static inline void unflatten_and_copy_device_tree(void) {}
>>  #endif /* CONFIG_OF_EARLY_FLATTREE */
>>  
>> +bool of_fdt_cells_size_fitted(u64 base, u64 size);
>> +size_t of_fdt_reg_cells_size(void);
>> +int fdt_prop_len(const char *prop_name, int len);
>> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
>> +						u64 addr, u64 size);
>> +
>>  #endif /* __ASSEMBLY__ */
>>  #endif /* _LINUX_OF_FDT_H */
>>
> 
> 

Re: [PATCH v14 06/16] of/fdt: add helper functions for handling properties

[Frank Rowand]

On 09/09/18 19:38, AKASHI Takahiro wrote:
> Frank,
> 
> Thank you for the comments. I will address all of them, but
> have one question:
> 
> On Fri, Sep 07, 2018 at 12:53:58PM -0700, Frank Rowand wrote:
>> On 09/07/18 01:00, AKASHI Takahiro wrote:
>>> These functions will be used later to handle kexec-specific properties
>>> in arm64's kexec_file implementation.
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> Cc: Rob Herring <robh+dt@kernel.org>
>>> Cc: Frank Rowand <frowand.list@gmail.com>
>>> ---
>>>  drivers/of/fdt.c       | 62 ++++++++++++++++++++++++++++++++++++++++--
>>>  include/linux/of_fdt.h | 10 +++++--
>>>  2 files changed, 68 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>>> index 800ad252cf9c..dc960cea1355 100644
>>> --- a/drivers/of/fdt.c
>>> +++ b/drivers/of/fdt.c
>>> @@ -25,6 +25,7 @@
>>>  #include <linux/debugfs.h>
>>>  #include <linux/serial_core.h>
>>>  #include <linux/sysfs.h>
>>> +#include <linux/types.h>
>>>  
>>>  #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
>>>  #include <asm/page.h>
>>> @@ -537,8 +538,8 @@ void *of_fdt_unflatten_tree(const unsigned long *blob,
>>>  EXPORT_SYMBOL_GPL(of_fdt_unflatten_tree);
>>>  
>>>  /* Everything below here references initial_boot_params directly. */
>>> -int __initdata dt_root_addr_cells;
>>> -int __initdata dt_root_size_cells;
>>> +int dt_root_addr_cells;
>>> +int dt_root_size_cells;
>>>  
>>>  void *initial_boot_params;
>>>  
>>> @@ -1323,3 +1324,60 @@ late_initcall(of_fdt_raw_init);
>>>  #endif
>>>  
>>>  #endif /* CONFIG_OF_EARLY_FLATTREE */
>>> +
>>
>> Please add comment:
>>
>> /* helper functions for arm64 kexec */
>>
>>
>>> +bool of_fdt_cells_size_fitted(u64 base, u64 size)
>>
>> Please rename as of_fdt_range_valid()
>>
>>
>>> +{
>>> +	/* if *_cells >= 2, cells can hold 64-bit values anyway */
>>> +	if ((dt_root_addr_cells == 1) && (base > U32_MAX))
>>> +		return false;
>>> +
>>> +	if ((dt_root_size_cells == 1) && (size > U32_MAX))
>>> +		return false;
>>
>> Should also check that base + size does not wrap around.
> 
> What is the upper limit here?
> For instance, #address_cells = <1> and #size_cells = <1>,
> and can 'base + size' be over U32_MAX?
> Assuming 'not' is quite reasonable, but it seems to me
> that devicetree spec doesn't exclude it, as least I couldn't
> find any notes about such a case.
> (In my understands, #address_cells only restricts a size in 'reg' property.)

(See my other reply in this thread a few minutes ago -- the context of the
ranges is essentially what is valid for the "reg" property of the "/memory"
nodes.)

The "Devicetree Specification" does not specify whether a memory range can
wrap around.  For example, can a region of size 0x2000 that wraps around
the highest 32 bit address be specified as:

   / memory@0xfffff000 {
        reg = <0xfffff000 0x2000>;
     }

or must it be specified as (option 1):

   / memory@0xfffff000 {
        reg = <0xfffff000 0x1000 0x0 0x1000>
     }

or (option 2):

   / memory@0xfffff000 {
        reg = <0xfffff000 0x1000>;
     }
     memory@0 {
        reg = <0x0 0x1000>;
     }


I suggest you start a thread on the devicetree specification list asking
that the spec be updated to state whether wrap around is allowed.


> 
> Thanks,
> -Takahiro Akashi
> 
>>
>>> +
>>> +	return true;
>>> +}
>>> +
>>> +size_t of_fdt_reg_cells_size(void)
>>
>> Please rename as of_fdt_root_range_size()
>>
>>
>>> +{
>>> +	return (dt_root_addr_cells + dt_root_size_cells) * sizeof(u32);
>>> +}
>>> +
>>> +#define FDT_ALIGN(x, a)	(((x) + (a) - 1) & ~((a) - 1))
>>> +#define FDT_TAGALIGN(x)	(FDT_ALIGN((x), FDT_TAGSIZE))
>>> +
>>> +int fdt_prop_len(const char *prop_name, int len)
>>
>> Please rename as fdt_len_added_prop()
>>
>>
>>> +{
>>> +	return (strlen(prop_name) + 1) +
>>> +		sizeof(struct fdt_property) +
>>> +		FDT_TAGALIGN(len);
>>> +}
>>> +
>>
>> Please add comment, something like:
>>
>> /* cells must be 1 or 2 */
>>
>>
>>> +static void fill_property(void *buf, u64 val64, int cells)
>>
>> Please rename as cpu64_to_fdt_cells()
>>
>> Thanks,
>>
>> Frank
>>
>>> +{
>>> +	__be32 val32;
>>> +
>>> +	while (cells) {
>>> +		val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
>>> +		memcpy(buf, &val32, sizeof(val32));
>>> +		buf += sizeof(val32);
>>> +	}
>>> +}
>>> +
>>> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
>>> +						u64 addr, u64 size)
>>> +{
>>> +	char buf[sizeof(__be32) * 2 * 2];
>>> +		/* assume dt_root_[addr|size]_cells <= 2 */
>>> +	void *prop;
>>> +	size_t buf_size;
>>> +
>>> +	buf_size = of_fdt_reg_cells_size();
>>> +	prop = buf;
>>> +
>>> +	fill_property(prop, addr, dt_root_addr_cells);
>>> +	prop += dt_root_addr_cells * sizeof(u32);
>>> +
>>> +	fill_property(prop, size, dt_root_size_cells);
>>> +
>>> +	return fdt_setprop(fdt, nodeoffset, name, buf, buf_size);
>>> +}
>>> diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
>>> index b9cd9ebdf9b9..9615d6142578 100644
>>> --- a/include/linux/of_fdt.h
>>> +++ b/include/linux/of_fdt.h
>>> @@ -37,8 +37,8 @@ extern void *of_fdt_unflatten_tree(const unsigned long *blob,
>>>  				   struct device_node **mynodes);
>>>  
>>>  /* TBD: Temporary export of fdt globals - remove when code fully merged */
>>> -extern int __initdata dt_root_addr_cells;
>>> -extern int __initdata dt_root_size_cells;
>>> +extern int dt_root_addr_cells;
>>> +extern int dt_root_size_cells;
>>>  extern void *initial_boot_params;
>>>  
>>>  extern char __dtb_start[];
>>> @@ -108,5 +108,11 @@ static inline void unflatten_device_tree(void) {}
>>>  static inline void unflatten_and_copy_device_tree(void) {}
>>>  #endif /* CONFIG_OF_EARLY_FLATTREE */
>>>  
>>> +bool of_fdt_cells_size_fitted(u64 base, u64 size);
>>> +size_t of_fdt_reg_cells_size(void);
>>> +int fdt_prop_len(const char *prop_name, int len);
>>> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
>>> +						u64 addr, u64 size);
>>> +
>>>  #endif /* __ASSEMBLY__ */
>>>  #endif /* _LINUX_OF_FDT_H */
>>>
>>
> 

Re: [PATCH v14 06/16] of/fdt: add helper functions for handling properties

[Frank Rowand]

On 09/13/18 18:26, Frank Rowand wrote:
> I was re-reading this while answering a later email in the thread.  After reading
> other patches in the series that were not sent to me, I have a better understanding
> of the intent behind this patch, and some changes to my previous reply.
> 
> The intent of the helper functions is related to properties whose values are
> tuples of the same format as the "reg" property of the "/memory" nodes.  For
> example, the "linux,usable-memory-range" and "linux,elfcoredhr" properties of
> the "/chosen" node.
> 
> The patch header and the function names should be updated to reflect this intent.
> This means most or all of my previous suggested function name changes are no longer
> useful.
> 
> Please add devicetree@vger.kernel.org to the next version of this patch and to
> the patches that use the functions in this patch.
> 
> 
> On 09/07/18 12:53, Frank Rowand wrote:
>> On 09/07/18 01:00, AKASHI Takahiro wrote:
>>> These functions will be used later to handle kexec-specific properties
>>> in arm64's kexec_file implementation.
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> Cc: Rob Herring <robh+dt@kernel.org>
>>> Cc: Frank Rowand <frowand.list@gmail.com>
>>> ---
>>>  drivers/of/fdt.c       | 62 ++++++++++++++++++++++++++++++++++++++++--
>>>  include/linux/of_fdt.h | 10 +++++--
>>>  2 files changed, 68 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>>> index 800ad252cf9c..dc960cea1355 100644
>>> --- a/drivers/of/fdt.c
>>> +++ b/drivers/of/fdt.c
>>> @@ -25,6 +25,7 @@
>>>  #include <linux/debugfs.h>
>>>  #include <linux/serial_core.h>
>>>  #include <linux/sysfs.h>
>>> +#include <linux/types.h>
>>>  
>>>  #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
>>>  #include <asm/page.h>
>>> @@ -537,8 +538,8 @@ void *of_fdt_unflatten_tree(const unsigned long *blob,
>>>  EXPORT_SYMBOL_GPL(of_fdt_unflatten_tree);
>>>  
>>>  /* Everything below here references initial_boot_params directly. */
>>> -int __initdata dt_root_addr_cells;
>>> -int __initdata dt_root_size_cells;
>>> +int dt_root_addr_cells;
>>> +int dt_root_size_cells;
>>>  
>>>  void *initial_boot_params;
>>>  
>>> @@ -1323,3 +1324,60 @@ late_initcall(of_fdt_raw_init);
>>>  #endif
>>>  
>>>  #endif /* CONFIG_OF_EARLY_FLATTREE */
>>> +
> 
> Global comment: this code should not be using the variables
> dt_root_addr_cells and dt_root_size_cells.  These variables are
> __initdata.
> 
> The code that is using these helpers is acting upon a specific FDT
> (copied from initial_boot_params).  This code should be getting the
> values of the root node's "#address-cells" and "#size-cells" from
> the FDT.

There will be new functions available soon to return the values of
a node's "#address-cells" and "#size-cells" from an fdt.  They are
fdt_address_cells() and fdt_size_cells().

Rob submitted the patch to add them yesterday in "[PATCH 3/3] scripts/dtc:
Update to upstream version v1.4.7-14-gc86da84d30e4" [1]

  [1] https://lkml.kernel.org/r/<20180913202828.15372-3-robh@kernel.org>

-Frank


> 
> 
>> Please add comment:
>>
>> /* helper functions for arm64 kexec */
>>
>>
>>> +bool of_fdt_cells_size_fitted(u64 base, u64 size)
>>
>> Please rename as of_fdt_range_valid()
> 
> I'm not entirely sure of what the caller in 12/16 is trying to ensure
> with this function.
> 
> (1) At the minimum (and what the implementation in of_fdt_cells_size_fitted()
> does) is make sure that an address and size tuple are consistent with
> the root properties "#address-cells" and "#size-cells".
> 
> The caller in 12/16 is using this check to validate values for the
> properties  "linux,elfcorehdr" and "linux,usable-memory-range".
> 
> (2) A more complete check _might_ be to ensure that the values also
> specify memory that is available to the kernel.  This memory is described
> by the "reg" property of one or more "/memory" nodes.
> 
> This second check is probably what is actually desired.
> 
> One possible issue to note is that the binding for "linux,usable-memory-range"
> suggests that available memory could be described by an EFI memory map.
> I am not familiar with how or when an EFI memory map might exist instead
> of the "/memory" nodes.
> 
> 
>>> +{
>>> +	/* if *_cells >= 2, cells can hold 64-bit values anyway */
>>> +	if ((dt_root_addr_cells == 1) && (base > U32_MAX))
>>> +		return false;
>>> +
>>> +	if ((dt_root_size_cells == 1) && (size > U32_MAX))
>>> +		return false;
>>
>> Should also check that base + size does not wrap around.
>>
>>
>>> +
>>> +	return true;
>>> +}
>>> +
>>> +size_t of_fdt_reg_cells_size(void)
>>
>> Please rename as of_fdt_root_range_size()
> 
> Even better would be to remove this function and replace the one place
> that it is called from with the one line of code in this function.
> 
> -Frank
> 
> 
>>> +{
>>> +	return (dt_root_addr_cells + dt_root_size_cells) * sizeof(u32);
>>> +}
>>> +
>>> +#define FDT_ALIGN(x, a)	(((x) + (a) - 1) & ~((a) - 1))
>>> +#define FDT_TAGALIGN(x)	(FDT_ALIGN((x), FDT_TAGSIZE))
>>> +
>>> +int fdt_prop_len(const char *prop_name, int len)
>>
>> Please rename as fdt_len_added_prop()
>>
>>
>>> +{
>>> +	return (strlen(prop_name) + 1) +
>>> +		sizeof(struct fdt_property) +
>>> +		FDT_TAGALIGN(len);
>>> +}
>>> +
>>
>> Please add comment, something like:
>>
>> /* cells must be 1 or 2 */
>>
>>
>>> +static void fill_property(void *buf, u64 val64, int cells)
>>
>> Please rename as cpu64_to_fdt_cells()
>>
>> Thanks,
>>
>> Frank
>>
>>> +{
>>> +	__be32 val32;
>>> +
>>> +	while (cells) {
>>> +		val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
>>> +		memcpy(buf, &val32, sizeof(val32));
>>> +		buf += sizeof(val32);
>>> +	}
>>> +}
>>> +
>>> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
>>> +						u64 addr, u64 size)
>>> +{
>>> +	char buf[sizeof(__be32) * 2 * 2];
>>> +		/* assume dt_root_[addr|size]_cells <= 2 */
>>> +	void *prop;
>>> +	size_t buf_size;
>>> +
>>> +	buf_size = of_fdt_reg_cells_size();
>>> +	prop = buf;
>>> +
>>> +	fill_property(prop, addr, dt_root_addr_cells);
>>> +	prop += dt_root_addr_cells * sizeof(u32);
>>> +
>>> +	fill_property(prop, size, dt_root_size_cells);
>>> +
>>> +	return fdt_setprop(fdt, nodeoffset, name, buf, buf_size);
>>> +}
>>> diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
>>> index b9cd9ebdf9b9..9615d6142578 100644
>>> --- a/include/linux/of_fdt.h
>>> +++ b/include/linux/of_fdt.h
>>> @@ -37,8 +37,8 @@ extern void *of_fdt_unflatten_tree(const unsigned long *blob,
>>>  				   struct device_node **mynodes);
>>>  
>>>  /* TBD: Temporary export of fdt globals - remove when code fully merged */
>>> -extern int __initdata dt_root_addr_cells;
>>> -extern int __initdata dt_root_size_cells;
>>> +extern int dt_root_addr_cells;
>>> +extern int dt_root_size_cells;
>>>  extern void *initial_boot_params;
>>>  
>>>  extern char __dtb_start[];
>>> @@ -108,5 +108,11 @@ static inline void unflatten_device_tree(void) {}
>>>  static inline void unflatten_and_copy_device_tree(void) {}
>>>  #endif /* CONFIG_OF_EARLY_FLATTREE */
>>>  
>>> +bool of_fdt_cells_size_fitted(u64 base, u64 size);
>>> +size_t of_fdt_reg_cells_size(void);
>>> +int fdt_prop_len(const char *prop_name, int len);
>>> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
>>> +						u64 addr, u64 size);
>>> +
>>>  #endif /* __ASSEMBLY__ */
>>>  #endif /* _LINUX_OF_FDT_H */
>>>
>>
>>
> 
> 

Re: [PATCH v14 06/16] of/fdt: add helper functions for handling properties

[AKASHI Takahiro]

Frank,

On Fri, Sep 14, 2018 at 10:19:38AM -0700, Frank Rowand wrote:
> On 09/13/18 18:26, Frank Rowand wrote:
> > I was re-reading this while answering a later email in the thread.  After reading
> > other patches in the series that were not sent to me, I have a better understanding
> > of the intent behind this patch, and some changes to my previous reply.
> > 
> > The intent of the helper functions is related to properties whose values are
> > tuples of the same format as the "reg" property of the "/memory" nodes.  For
> > example, the "linux,usable-memory-range" and "linux,elfcoredhr" properties of
> > the "/chosen" node.
> > 
> > The patch header and the function names should be updated to reflect this intent.
> > This means most or all of my previous suggested function name changes are no longer
> > useful.
> > 
> > Please add devicetree@vger.kernel.org to the next version of this patch and to
> > the patches that use the functions in this patch.
> > 
> > 
> > On 09/07/18 12:53, Frank Rowand wrote:
> >> On 09/07/18 01:00, AKASHI Takahiro wrote:
> >>> These functions will be used later to handle kexec-specific properties
> >>> in arm64's kexec_file implementation.
> >>>
> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>> Cc: Rob Herring <robh+dt@kernel.org>
> >>> Cc: Frank Rowand <frowand.list@gmail.com>
> >>> ---
> >>>  drivers/of/fdt.c       | 62 ++++++++++++++++++++++++++++++++++++++++--
> >>>  include/linux/of_fdt.h | 10 +++++--
> >>>  2 files changed, 68 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> >>> index 800ad252cf9c..dc960cea1355 100644
> >>> --- a/drivers/of/fdt.c
> >>> +++ b/drivers/of/fdt.c
> >>> @@ -25,6 +25,7 @@
> >>>  #include <linux/debugfs.h>
> >>>  #include <linux/serial_core.h>
> >>>  #include <linux/sysfs.h>
> >>> +#include <linux/types.h>
> >>>  
> >>>  #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
> >>>  #include <asm/page.h>
> >>> @@ -537,8 +538,8 @@ void *of_fdt_unflatten_tree(const unsigned long *blob,
> >>>  EXPORT_SYMBOL_GPL(of_fdt_unflatten_tree);
> >>>  
> >>>  /* Everything below here references initial_boot_params directly. */
> >>> -int __initdata dt_root_addr_cells;
> >>> -int __initdata dt_root_size_cells;
> >>> +int dt_root_addr_cells;
> >>> +int dt_root_size_cells;
> >>>  
> >>>  void *initial_boot_params;
> >>>  
> >>> @@ -1323,3 +1324,60 @@ late_initcall(of_fdt_raw_init);
> >>>  #endif
> >>>  
> >>>  #endif /* CONFIG_OF_EARLY_FLATTREE */
> >>> +
> > 
> > Global comment: this code should not be using the variables
> > dt_root_addr_cells and dt_root_size_cells.  These variables are
> > __initdata.
> > 
> > The code that is using these helpers is acting upon a specific FDT
> > (copied from initial_boot_params).  This code should be getting the
> > values of the root node's "#address-cells" and "#size-cells" from
> > the FDT.
> 
> There will be new functions available soon to return the values of
> a node's "#address-cells" and "#size-cells" from an fdt.  They are
> fdt_address_cells() and fdt_size_cells().
> 
> Rob submitted the patch to add them yesterday in "[PATCH 3/3] scripts/dtc:
> Update to upstream version v1.4.7-14-gc86da84d30e4" [1]

Will this patch go into mainline in v4.20 merge window?

>   [1] https://lkml.kernel.org/r/<20180913202828.15372-3-robh@kernel.org>

Unfortunately, fdt_addresses.c where fdt_address_cells() and
fdt_size_cells() are defined is NOT compiled in the kernel.
I will submit a patch.

-Takahiro Akashi


> -Frank
> 
> 
> > 
> > 
> >> Please add comment:
> >>
> >> /* helper functions for arm64 kexec */
> >>
> >>
> >>> +bool of_fdt_cells_size_fitted(u64 base, u64 size)
> >>
> >> Please rename as of_fdt_range_valid()
> > 
> > I'm not entirely sure of what the caller in 12/16 is trying to ensure
> > with this function.
> > 
> > (1) At the minimum (and what the implementation in of_fdt_cells_size_fitted()
> > does) is make sure that an address and size tuple are consistent with
> > the root properties "#address-cells" and "#size-cells".
> > 
> > The caller in 12/16 is using this check to validate values for the
> > properties  "linux,elfcorehdr" and "linux,usable-memory-range".
> > 
> > (2) A more complete check _might_ be to ensure that the values also
> > specify memory that is available to the kernel.  This memory is described
> > by the "reg" property of one or more "/memory" nodes.
> > 
> > This second check is probably what is actually desired.
> > 
> > One possible issue to note is that the binding for "linux,usable-memory-range"
> > suggests that available memory could be described by an EFI memory map.
> > I am not familiar with how or when an EFI memory map might exist instead
> > of the "/memory" nodes.
> > 
> > 
> >>> +{
> >>> +	/* if *_cells >= 2, cells can hold 64-bit values anyway */
> >>> +	if ((dt_root_addr_cells == 1) && (base > U32_MAX))
> >>> +		return false;
> >>> +
> >>> +	if ((dt_root_size_cells == 1) && (size > U32_MAX))
> >>> +		return false;
> >>
> >> Should also check that base + size does not wrap around.
> >>
> >>
> >>> +
> >>> +	return true;
> >>> +}
> >>> +
> >>> +size_t of_fdt_reg_cells_size(void)
> >>
> >> Please rename as of_fdt_root_range_size()
> > 
> > Even better would be to remove this function and replace the one place
> > that it is called from with the one line of code in this function.
> > 
> > -Frank
> > 
> > 
> >>> +{
> >>> +	return (dt_root_addr_cells + dt_root_size_cells) * sizeof(u32);
> >>> +}
> >>> +
> >>> +#define FDT_ALIGN(x, a)	(((x) + (a) - 1) & ~((a) - 1))
> >>> +#define FDT_TAGALIGN(x)	(FDT_ALIGN((x), FDT_TAGSIZE))
> >>> +
> >>> +int fdt_prop_len(const char *prop_name, int len)
> >>
> >> Please rename as fdt_len_added_prop()
> >>
> >>
> >>> +{
> >>> +	return (strlen(prop_name) + 1) +
> >>> +		sizeof(struct fdt_property) +
> >>> +		FDT_TAGALIGN(len);
> >>> +}
> >>> +
> >>
> >> Please add comment, something like:
> >>
> >> /* cells must be 1 or 2 */
> >>
> >>
> >>> +static void fill_property(void *buf, u64 val64, int cells)
> >>
> >> Please rename as cpu64_to_fdt_cells()
> >>
> >> Thanks,
> >>
> >> Frank
> >>
> >>> +{
> >>> +	__be32 val32;
> >>> +
> >>> +	while (cells) {
> >>> +		val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
> >>> +		memcpy(buf, &val32, sizeof(val32));
> >>> +		buf += sizeof(val32);
> >>> +	}
> >>> +}
> >>> +
> >>> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
> >>> +						u64 addr, u64 size)
> >>> +{
> >>> +	char buf[sizeof(__be32) * 2 * 2];
> >>> +		/* assume dt_root_[addr|size]_cells <= 2 */
> >>> +	void *prop;
> >>> +	size_t buf_size;
> >>> +
> >>> +	buf_size = of_fdt_reg_cells_size();
> >>> +	prop = buf;
> >>> +
> >>> +	fill_property(prop, addr, dt_root_addr_cells);
> >>> +	prop += dt_root_addr_cells * sizeof(u32);
> >>> +
> >>> +	fill_property(prop, size, dt_root_size_cells);
> >>> +
> >>> +	return fdt_setprop(fdt, nodeoffset, name, buf, buf_size);
> >>> +}
> >>> diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
> >>> index b9cd9ebdf9b9..9615d6142578 100644
> >>> --- a/include/linux/of_fdt.h
> >>> +++ b/include/linux/of_fdt.h
> >>> @@ -37,8 +37,8 @@ extern void *of_fdt_unflatten_tree(const unsigned long *blob,
> >>>  				   struct device_node **mynodes);
> >>>  
> >>>  /* TBD: Temporary export of fdt globals - remove when code fully merged */
> >>> -extern int __initdata dt_root_addr_cells;
> >>> -extern int __initdata dt_root_size_cells;
> >>> +extern int dt_root_addr_cells;
> >>> +extern int dt_root_size_cells;
> >>>  extern void *initial_boot_params;
> >>>  
> >>>  extern char __dtb_start[];
> >>> @@ -108,5 +108,11 @@ static inline void unflatten_device_tree(void) {}
> >>>  static inline void unflatten_and_copy_device_tree(void) {}
> >>>  #endif /* CONFIG_OF_EARLY_FLATTREE */
> >>>  
> >>> +bool of_fdt_cells_size_fitted(u64 base, u64 size);
> >>> +size_t of_fdt_reg_cells_size(void);
> >>> +int fdt_prop_len(const char *prop_name, int len);
> >>> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
> >>> +						u64 addr, u64 size);
> >>> +
> >>>  #endif /* __ASSEMBLY__ */
> >>>  #endif /* _LINUX_OF_FDT_H */
> >>>
> >>
> >>
> > 
> > 
> 

Re: [PATCH v14 06/16] of/fdt: add helper functions for handling properties

[Frank Rowand]

Hi Rob,

On 09/25/18 22:57, AKASHI Takahiro wrote:
> Frank,
> 
> On Fri, Sep 14, 2018 at 10:19:38AM -0700, Frank Rowand wrote:
>> On 09/13/18 18:26, Frank Rowand wrote:
>>> I was re-reading this while answering a later email in the thread.  After reading
>>> other patches in the series that were not sent to me, I have a better understanding
>>> of the intent behind this patch, and some changes to my previous reply.
>>>
>>> The intent of the helper functions is related to properties whose values are
>>> tuples of the same format as the "reg" property of the "/memory" nodes.  For
>>> example, the "linux,usable-memory-range" and "linux,elfcoredhr" properties of
>>> the "/chosen" node.
>>>
>>> The patch header and the function names should be updated to reflect this intent.
>>> This means most or all of my previous suggested function name changes are no longer
>>> useful.
>>>
>>> Please add devicetree@vger.kernel.org to the next version of this patch and to
>>> the patches that use the functions in this patch.
>>>
>>>
>>> On 09/07/18 12:53, Frank Rowand wrote:
>>>> On 09/07/18 01:00, AKASHI Takahiro wrote:
>>>>> These functions will be used later to handle kexec-specific properties
>>>>> in arm64's kexec_file implementation.
>>>>>
>>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>>> Cc: Frank Rowand <frowand.list@gmail.com>
>>>>> ---
>>>>>  drivers/of/fdt.c       | 62 ++++++++++++++++++++++++++++++++++++++++--
>>>>>  include/linux/of_fdt.h | 10 +++++--
>>>>>  2 files changed, 68 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>>>>> index 800ad252cf9c..dc960cea1355 100644
>>>>> --- a/drivers/of/fdt.c
>>>>> +++ b/drivers/of/fdt.c
>>>>> @@ -25,6 +25,7 @@
>>>>>  #include <linux/debugfs.h>
>>>>>  #include <linux/serial_core.h>
>>>>>  #include <linux/sysfs.h>
>>>>> +#include <linux/types.h>
>>>>>  
>>>>>  #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
>>>>>  #include <asm/page.h>
>>>>> @@ -537,8 +538,8 @@ void *of_fdt_unflatten_tree(const unsigned long *blob,
>>>>>  EXPORT_SYMBOL_GPL(of_fdt_unflatten_tree);
>>>>>  
>>>>>  /* Everything below here references initial_boot_params directly. */
>>>>> -int __initdata dt_root_addr_cells;
>>>>> -int __initdata dt_root_size_cells;
>>>>> +int dt_root_addr_cells;
>>>>> +int dt_root_size_cells;
>>>>>  
>>>>>  void *initial_boot_params;
>>>>>  
>>>>> @@ -1323,3 +1324,60 @@ late_initcall(of_fdt_raw_init);
>>>>>  #endif
>>>>>  
>>>>>  #endif /* CONFIG_OF_EARLY_FLATTREE */
>>>>> +
>>>
>>> Global comment: this code should not be using the variables
>>> dt_root_addr_cells and dt_root_size_cells.  These variables are
>>> __initdata.
>>>
>>> The code that is using these helpers is acting upon a specific FDT
>>> (copied from initial_boot_params).  This code should be getting the
>>> values of the root node's "#address-cells" and "#size-cells" from
>>> the FDT.
>>


Can you confirm whether "[PATCH 3/3] scripts/dtc: ..." (see below) will
be in the v4.20 pull request?


>> There will be new functions available soon to return the values of
>> a node's "#address-cells" and "#size-cells" from an fdt.  They are
>> fdt_address_cells() and fdt_size_cells().
>>
>> Rob submitted the patch to add them yesterday in "[PATCH 3/3] scripts/dtc:
>> Update to upstream version v1.4.7-14-gc86da84d30e4" [1]
> 
> Will this patch go into mainline in v4.20 merge window?
> 
>>   [1] https://lkml.kernel.org/r/<20180913202828.15372-3-robh@kernel.org>
> 
> Unfortunately, fdt_addresses.c where fdt_address_cells() and
> fdt_size_cells() are defined is NOT compiled in the kernel.
> I will submit a patch.
> 
> -Takahiro Akashi
> 
> 
>> -Frank

Thanks,

Frank


>>
>>
>>>
>>>
>>>> Please add comment:
>>>>
>>>> /* helper functions for arm64 kexec */
>>>>
>>>>
>>>>> +bool of_fdt_cells_size_fitted(u64 base, u64 size)
>>>>
>>>> Please rename as of_fdt_range_valid()
>>>
>>> I'm not entirely sure of what the caller in 12/16 is trying to ensure
>>> with this function.
>>>
>>> (1) At the minimum (and what the implementation in of_fdt_cells_size_fitted()
>>> does) is make sure that an address and size tuple are consistent with
>>> the root properties "#address-cells" and "#size-cells".
>>>
>>> The caller in 12/16 is using this check to validate values for the
>>> properties  "linux,elfcorehdr" and "linux,usable-memory-range".
>>>
>>> (2) A more complete check _might_ be to ensure that the values also
>>> specify memory that is available to the kernel.  This memory is described
>>> by the "reg" property of one or more "/memory" nodes.
>>>
>>> This second check is probably what is actually desired.
>>>
>>> One possible issue to note is that the binding for "linux,usable-memory-range"
>>> suggests that available memory could be described by an EFI memory map.
>>> I am not familiar with how or when an EFI memory map might exist instead
>>> of the "/memory" nodes.
>>>
>>>
>>>>> +{
>>>>> +	/* if *_cells >= 2, cells can hold 64-bit values anyway */
>>>>> +	if ((dt_root_addr_cells == 1) && (base > U32_MAX))
>>>>> +		return false;
>>>>> +
>>>>> +	if ((dt_root_size_cells == 1) && (size > U32_MAX))
>>>>> +		return false;
>>>>
>>>> Should also check that base + size does not wrap around.
>>>>
>>>>
>>>>> +
>>>>> +	return true;
>>>>> +}
>>>>> +
>>>>> +size_t of_fdt_reg_cells_size(void)
>>>>
>>>> Please rename as of_fdt_root_range_size()
>>>
>>> Even better would be to remove this function and replace the one place
>>> that it is called from with the one line of code in this function.
>>>
>>> -Frank
>>>
>>>
>>>>> +{
>>>>> +	return (dt_root_addr_cells + dt_root_size_cells) * sizeof(u32);
>>>>> +}
>>>>> +
>>>>> +#define FDT_ALIGN(x, a)	(((x) + (a) - 1) & ~((a) - 1))
>>>>> +#define FDT_TAGALIGN(x)	(FDT_ALIGN((x), FDT_TAGSIZE))
>>>>> +
>>>>> +int fdt_prop_len(const char *prop_name, int len)
>>>>
>>>> Please rename as fdt_len_added_prop()
>>>>
>>>>
>>>>> +{
>>>>> +	return (strlen(prop_name) + 1) +
>>>>> +		sizeof(struct fdt_property) +
>>>>> +		FDT_TAGALIGN(len);
>>>>> +}
>>>>> +
>>>>
>>>> Please add comment, something like:
>>>>
>>>> /* cells must be 1 or 2 */
>>>>
>>>>
>>>>> +static void fill_property(void *buf, u64 val64, int cells)
>>>>
>>>> Please rename as cpu64_to_fdt_cells()
>>>>
>>>> Thanks,
>>>>
>>>> Frank
>>>>
>>>>> +{
>>>>> +	__be32 val32;
>>>>> +
>>>>> +	while (cells) {
>>>>> +		val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
>>>>> +		memcpy(buf, &val32, sizeof(val32));
>>>>> +		buf += sizeof(val32);
>>>>> +	}
>>>>> +}
>>>>> +
>>>>> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
>>>>> +						u64 addr, u64 size)
>>>>> +{
>>>>> +	char buf[sizeof(__be32) * 2 * 2];
>>>>> +		/* assume dt_root_[addr|size]_cells <= 2 */
>>>>> +	void *prop;
>>>>> +	size_t buf_size;
>>>>> +
>>>>> +	buf_size = of_fdt_reg_cells_size();
>>>>> +	prop = buf;
>>>>> +
>>>>> +	fill_property(prop, addr, dt_root_addr_cells);
>>>>> +	prop += dt_root_addr_cells * sizeof(u32);
>>>>> +
>>>>> +	fill_property(prop, size, dt_root_size_cells);
>>>>> +
>>>>> +	return fdt_setprop(fdt, nodeoffset, name, buf, buf_size);
>>>>> +}
>>>>> diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
>>>>> index b9cd9ebdf9b9..9615d6142578 100644
>>>>> --- a/include/linux/of_fdt.h
>>>>> +++ b/include/linux/of_fdt.h
>>>>> @@ -37,8 +37,8 @@ extern void *of_fdt_unflatten_tree(const unsigned long *blob,
>>>>>  				   struct device_node **mynodes);
>>>>>  
>>>>>  /* TBD: Temporary export of fdt globals - remove when code fully merged */
>>>>> -extern int __initdata dt_root_addr_cells;
>>>>> -extern int __initdata dt_root_size_cells;
>>>>> +extern int dt_root_addr_cells;
>>>>> +extern int dt_root_size_cells;
>>>>>  extern void *initial_boot_params;
>>>>>  
>>>>>  extern char __dtb_start[];
>>>>> @@ -108,5 +108,11 @@ static inline void unflatten_device_tree(void) {}
>>>>>  static inline void unflatten_and_copy_device_tree(void) {}
>>>>>  #endif /* CONFIG_OF_EARLY_FLATTREE */
>>>>>  
>>>>> +bool of_fdt_cells_size_fitted(u64 base, u64 size);
>>>>> +size_t of_fdt_reg_cells_size(void);
>>>>> +int fdt_prop_len(const char *prop_name, int len);
>>>>> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
>>>>> +						u64 addr, u64 size);
>>>>> +
>>>>>  #endif /* __ASSEMBLY__ */
>>>>>  #endif /* _LINUX_OF_FDT_H */
>>>>>
>>>>
>>>>
>>>
>>>
>>
>