[PATCH v4 0/2] arm64: kexec_file: add kdump

[PATCH v4 0/2] arm64: kexec_file: add kdump

[AKASHI Takahiro]

This is the last piece of my kexec_file_load implementation for arm64.
It is now ready for being merged as some relevant patch to dtc/libfdt[1]
has finally been integrated in v5.3-rc1.
(Nothing changed since kexec_file v16[2] except adding Patch#1 and #2.)

Patch#1 and #2 are preliminary patches for libfdt component.
Patch#3 is to add kdump support.

Bhepesh's patch[3] will be required for 52-bit VA support either against
legacy kexec or kexec_file.
Once this patch is applied, whether or not CONFIG_ARM64_VA_BITS_52 is
enabled or not, a matching fix[4] on user space side, crash utility,
will also be needed. 

Anyway, I tested my patch, at least, with the following configuration:
1) CONFIG_ARM64_BITS_48=y
2) CONFIG_ARM64_BITS_52=y, but vabits_actual=48

(I don't have any platform to use for
3) CONFIG_ARM64_BITS_52=y, and vabits_actual=52)

[1] commit 9bb9c6a110ea ("scripts/dtc: Update to upstream version
    v1.5.0-23-g87963ee20693"), in particular
	7fcf8208b8a9 libfdt: add fdt_append_addrrange()
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-November/612641.html
[3] http://lists.infradead.org/pipermail/linux-arm-kernel/2019-November/693411.html
[4] https://www.redhat.com/archives/crash-utility/2019-November/msg00014.html

Changes in v4 (Dec 16, 2019)
* rebased to v5.5-rc2
* use KEXEC_BUF_MEM_UNKNOWN instead of "0" (patch#2)

Changes in v3 (Dec 9, 2019)
* rebased to v5.5-rc1
* no functional changes (since v1)

Changes in v2 (Nov 14, 2019)
* rebased to v5.4-rc7
* no functional changes

v1 (Sept 12, 2019)
* on top of v5.3-rc8

AKASHI Takahiro (2):
  libfdt: include fdt_addresses.c
  arm64: kexec_file: add crash dump support

 arch/arm64/include/asm/kexec.h         |   4 +
 arch/arm64/kernel/kexec_image.c        |   4 -
 arch/arm64/kernel/machine_kexec_file.c | 106 ++++++++++++++++++++++++-
 lib/Makefile                           |   2 +-
 lib/fdt_addresses.c                    |   2 +
 5 files changed, 109 insertions(+), 9 deletions(-)
 create mode 100644 lib/fdt_addresses.c

-- 
2.24.0

[PATCH v4 1/2] libfdt: include fdt_addresses.c

[AKASHI Takahiro]

In the implementation of kexec_file_loaded-based kdump for arm64,
fdt_appendprop_addrrange() will be needed.

So include fdt_addresses.c in making libfdt.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
---
 lib/Makefile        | 2 +-
 lib/fdt_addresses.c | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)
 create mode 100644 lib/fdt_addresses.c

diff --git a/lib/Makefile b/lib/Makefile
index 93217d44237f..c20b1debe9b4 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -223,7 +223,7 @@ KASAN_SANITIZE_stackdepot.o := n
 KCOV_INSTRUMENT_stackdepot.o := n
 
 libfdt_files = fdt.o fdt_ro.o fdt_wip.o fdt_rw.o fdt_sw.o fdt_strerror.o \
-	       fdt_empty_tree.o
+	       fdt_empty_tree.o fdt_addresses.o
 $(foreach file, $(libfdt_files), \
 	$(eval CFLAGS_$(file) = -I $(srctree)/scripts/dtc/libfdt))
 lib-$(CONFIG_LIBFDT) += $(libfdt_files)
diff --git a/lib/fdt_addresses.c b/lib/fdt_addresses.c
new file mode 100644
index 000000000000..23610bcf390b
--- /dev/null
+++ b/lib/fdt_addresses.c
@@ -0,0 +1,2 @@
+#include <linux/libfdt_env.h>
+#include "../scripts/dtc/libfdt/fdt_addresses.c"
-- 
2.24.0

[PATCH v4 2/2] 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>
Tested-and-reviewed-by: Bhupesh Sharma <bhsharma@redhat.com>
---
 arch/arm64/include/asm/kexec.h         |   4 +
 arch/arm64/kernel/kexec_image.c        |   4 -
 arch/arm64/kernel/machine_kexec_file.c | 106 ++++++++++++++++++++++++-
 3 files changed, 106 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
index 12a561a54128..d24b527e8c00 100644
--- a/arch/arm64/include/asm/kexec.h
+++ b/arch/arm64/include/asm/kexec.h
@@ -96,6 +96,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;
 };
 
 extern const struct kexec_file_ops kexec_image_ops;
diff --git a/arch/arm64/kernel/kexec_image.c b/arch/arm64/kernel/kexec_image.c
index 29a9428486a5..af9987c154ca 100644
--- a/arch/arm64/kernel/kexec_image.c
+++ b/arch/arm64/kernel/kexec_image.c
@@ -47,10 +47,6 @@ static void *image_load(struct kimage *image,
 	struct kexec_segment *kernel_segment;
 	int ret;
 
-	/* We don't support crash kernels yet. */
-	if (image->type == KEXEC_TYPE_CRASH)
-		return ERR_PTR(-EOPNOTSUPP);
-
 	/*
 	 * We require a kernel with an unambiguous Image header. Per
 	 * Documentation/arm64/booting.rst, this is the case when image_size
diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c
index 7b08bf9499b6..dd3ae8081b38 100644
--- a/arch/arm64/kernel/machine_kexec_file.c
+++ b/arch/arm64/kernel/machine_kexec_file.c
@@ -17,12 +17,15 @@
 #include <linux/memblock.h>
 #include <linux/of_fdt.h>
 #include <linux/random.h>
+#include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/types.h>
 #include <linux/vmalloc.h>
 #include <asm/byteorder.h>
 
 /* relevant device tree properties */
+#define FDT_PROP_KEXEC_ELFHDR	"linux,elfcorehdr"
+#define FDT_PROP_MEM_RANGE	"linux,usable-memory-range"
 #define FDT_PROP_INITRD_START	"linux,initrd-start"
 #define FDT_PROP_INITRD_END	"linux,initrd-end"
 #define FDT_PROP_BOOTARGS	"bootargs"
@@ -40,6 +43,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);
 }
 
@@ -55,6 +62,31 @@ static int setup_dtb(struct kimage *image,
 
 	off = ret;
 
+	ret = fdt_delprop(dtb, off, FDT_PROP_KEXEC_ELFHDR);
+	if (ret && ret != -FDT_ERR_NOTFOUND)
+		goto out;
+	ret = fdt_delprop(dtb, off, FDT_PROP_MEM_RANGE);
+	if (ret && ret != -FDT_ERR_NOTFOUND)
+		goto out;
+
+	if (image->type == KEXEC_TYPE_CRASH) {
+		/* add linux,elfcorehdr */
+		ret = fdt_appendprop_addrrange(dtb, 0, off,
+				FDT_PROP_KEXEC_ELFHDR,
+				image->arch.elf_headers_mem,
+				image->arch.elf_headers_sz);
+		if (ret)
+			return (ret == -FDT_ERR_NOSPACE ? -ENOMEM : -EINVAL);
+
+		/* add linux,usable-memory-range */
+		ret = fdt_appendprop_addrrange(dtb, 0, off,
+				FDT_PROP_MEM_RANGE,
+				crashk_res.start,
+				crashk_res.end - crashk_res.start + 1);
+		if (ret)
+			return (ret == -FDT_ERR_NOSPACE ? -ENOMEM : -EINVAL);
+	}
+
 	/* add bootargs */
 	if (cmdline) {
 		ret = fdt_setprop_string(dtb, off, FDT_PROP_BOOTARGS, cmdline);
@@ -125,8 +157,8 @@ static int setup_dtb(struct kimage *image,
 }
 
 /*
- * More space needed so that we can add initrd, bootargs, kaslr-seed, and
- * rng-seed.
+ * More space needed so that we can add initrd, bootargs, kaslr-seed,
+ * rng-seed, userable-memory-range and elfcorehdr.
  */
 #define DTB_EXTRA_SPACE 0x1000
 
@@ -174,6 +206,43 @@ static int create_dtb(struct kimage *image,
 	}
 }
 
+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,
@@ -181,14 +250,43 @@ int load_other_segments(struct kimage *image,
 			char *cmdline)
 {
 	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 = KEXEC_BUF_MEM_UNKNOWN;
+		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.24.0

Re: [PATCH v4 2/2] arm64: kexec_file: add crash dump support

[Will Deacon]

On Mon, Dec 16, 2019 at 11:12:47AM +0900, AKASHI Takahiro wrote:
> 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>
> Tested-and-reviewed-by: Bhupesh Sharma <bhsharma@redhat.com>
> ---
>  arch/arm64/include/asm/kexec.h         |   4 +
>  arch/arm64/kernel/kexec_image.c        |   4 -
>  arch/arm64/kernel/machine_kexec_file.c | 106 ++++++++++++++++++++++++-
>  3 files changed, 106 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
> index 12a561a54128..d24b527e8c00 100644
> --- a/arch/arm64/include/asm/kexec.h
> +++ b/arch/arm64/include/asm/kexec.h
> @@ -96,6 +96,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;
>  };

This conflicts with the cleanup work from Pavel. Please can you check my
resolution? [1]

Will

[1] https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/diff/?h=for-kernelci&id=aef73191765a88cadc0a627cdc070e5a0086b015

Re: [PATCH v4 2/2] arm64: kexec_file: add crash dump support

[Pavel Tatashin]

Looks good to me.

Pasha

On Wed, Jan 8, 2020 at 12:48 PM Will Deacon <will@kernel.org> wrote:
>
> On Mon, Dec 16, 2019 at 11:12:47AM +0900, AKASHI Takahiro wrote:
> > 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>
> > Tested-and-reviewed-by: Bhupesh Sharma <bhsharma@redhat.com>
> > ---
> >  arch/arm64/include/asm/kexec.h         |   4 +
> >  arch/arm64/kernel/kexec_image.c        |   4 -
> >  arch/arm64/kernel/machine_kexec_file.c | 106 ++++++++++++++++++++++++-
> >  3 files changed, 106 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
> > index 12a561a54128..d24b527e8c00 100644
> > --- a/arch/arm64/include/asm/kexec.h
> > +++ b/arch/arm64/include/asm/kexec.h
> > @@ -96,6 +96,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;
> >  };
>
> This conflicts with the cleanup work from Pavel. Please can you check my
> resolution? [1]
>
> Will
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/diff/?h=for-kernelci&id=aef73191765a88cadc0a627cdc070e5a0086b015
>

Re: [PATCH v4 2/2] arm64: kexec_file: add crash dump support

[AKASHI Takahiro]

On Wed, Jan 08, 2020 at 05:48:39PM +0000, Will Deacon wrote:
> On Mon, Dec 16, 2019 at 11:12:47AM +0900, AKASHI Takahiro wrote:
> > 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>
> > Tested-and-reviewed-by: Bhupesh Sharma <bhsharma@redhat.com>
> > ---
> >  arch/arm64/include/asm/kexec.h         |   4 +
> >  arch/arm64/kernel/kexec_image.c        |   4 -
> >  arch/arm64/kernel/machine_kexec_file.c | 106 ++++++++++++++++++++++++-
> >  3 files changed, 106 insertions(+), 8 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
> > index 12a561a54128..d24b527e8c00 100644
> > --- a/arch/arm64/include/asm/kexec.h
> > +++ b/arch/arm64/include/asm/kexec.h
> > @@ -96,6 +96,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;
> >  };
> 
> This conflicts with the cleanup work from Pavel. Please can you check my
> resolution? [1]

I don't know why we need to change a type of dtb_mem,
otherwise it looks good.

(I also assume that you notice that kimage_arch is of no use for kexec.)

Thank you for the merge,
-Takahiro Akashi


> Will
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/diff/?h=for-kernelci&id=aef73191765a88cadc0a627cdc070e5a0086b015
>

Re: [PATCH v4 2/2] arm64: kexec_file: add crash dump support

[Will Deacon]

On Thu, Jan 09, 2020 at 09:46:55AM +0900, AKASHI Takahiro wrote:
> On Wed, Jan 08, 2020 at 05:48:39PM +0000, Will Deacon wrote:
> > On Mon, Dec 16, 2019 at 11:12:47AM +0900, AKASHI Takahiro wrote:
> > > 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>
> > > Tested-and-reviewed-by: Bhupesh Sharma <bhsharma@redhat.com>
> > > ---
> > >  arch/arm64/include/asm/kexec.h         |   4 +
> > >  arch/arm64/kernel/kexec_image.c        |   4 -
> > >  arch/arm64/kernel/machine_kexec_file.c | 106 ++++++++++++++++++++++++-
> > >  3 files changed, 106 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
> > > index 12a561a54128..d24b527e8c00 100644
> > > --- a/arch/arm64/include/asm/kexec.h
> > > +++ b/arch/arm64/include/asm/kexec.h
> > > @@ -96,6 +96,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;
> > >  };
> > 
> > This conflicts with the cleanup work from Pavel. Please can you check my
> > resolution? [1]
> 
> I don't know why we need to change a type of dtb_mem,
> otherwise it looks good.
> 
> (I also assume that you notice that kimage_arch is of no use for kexec.)

Yes, that's why I'd like the resolution checked. If you reckon it's cleaner
to drop Pavel's patch altogether in light of your changes, we can do that
instead.

Thoughts?

Will

Re: [PATCH v4 2/2] arm64: kexec_file: add crash dump support

[Will Deacon]

On Thu, Jan 09, 2020 at 08:32:54AM +0000, Will Deacon wrote:
> On Thu, Jan 09, 2020 at 09:46:55AM +0900, AKASHI Takahiro wrote:
> > On Wed, Jan 08, 2020 at 05:48:39PM +0000, Will Deacon wrote:
> > > On Mon, Dec 16, 2019 at 11:12:47AM +0900, AKASHI Takahiro wrote:
> > > > diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
> > > > index 12a561a54128..d24b527e8c00 100644
> > > > --- a/arch/arm64/include/asm/kexec.h
> > > > +++ b/arch/arm64/include/asm/kexec.h
> > > > @@ -96,6 +96,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;
> > > >  };
> > > 
> > > This conflicts with the cleanup work from Pavel. Please can you check my
> > > resolution? [1]
> > 
> > I don't know why we need to change a type of dtb_mem,
> > otherwise it looks good.
> > 
> > (I also assume that you notice that kimage_arch is of no use for kexec.)
> 
> Yes, that's why I'd like the resolution checked. If you reckon it's cleaner
> to drop Pavel's patch altogether in light of your changes, we can do that
> instead.
> 
> Thoughts?

Well, I've reverted the cleanup patch so please shout if you'd prefer
something else.

Will

Re: [PATCH v4 2/2] arm64: kexec_file: add crash dump support

[Pavel Tatashin]

On Fri, Jan 10, 2020 at 11:05 AM Will Deacon <will@kernel.org> wrote:
>
> On Thu, Jan 09, 2020 at 08:32:54AM +0000, Will Deacon wrote:
> > On Thu, Jan 09, 2020 at 09:46:55AM +0900, AKASHI Takahiro wrote:
> > > On Wed, Jan 08, 2020 at 05:48:39PM +0000, Will Deacon wrote:
> > > > On Mon, Dec 16, 2019 at 11:12:47AM +0900, AKASHI Takahiro wrote:
> > > > > diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
> > > > > index 12a561a54128..d24b527e8c00 100644
> > > > > --- a/arch/arm64/include/asm/kexec.h
> > > > > +++ b/arch/arm64/include/asm/kexec.h
> > > > > @@ -96,6 +96,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;
> > > > >  };
> > > >
> > > > This conflicts with the cleanup work from Pavel. Please can you check my
> > > > resolution? [1]
> > >
> > > I don't know why we need to change a type of dtb_mem,
> > > otherwise it looks good.
> > >
> > > (I also assume that you notice that kimage_arch is of no use for kexec.)
> >
> > Yes, that's why I'd like the resolution checked. If you reckon it's cleaner
> > to drop Pavel's patch altogether in light of your changes, we can do that
> > instead.
> >
> > Thoughts?
>
> Well, I've reverted the cleanup patch so please shout if you'd prefer
> something else.

As I understand, the only concern was the type change for dtb_mem.
This was one of the review comments for my patch
https://lore.kernel.org/lkml/20191204155938.2279686-21-pasha.tatashin@soleen.com/

(I believe it was from Marc Zyngier), I add a number of new fields,
and they all should be phys_addr_t, this is why I change dtb_mem to
phys_addr_t to be consistent.

Pasha

Re: [PATCH v4 2/2] arm64: kexec_file: add crash dump support

[Will Deacon]

On Fri, Jan 10, 2020 at 11:19:16AM -0500, Pavel Tatashin wrote:
> On Fri, Jan 10, 2020 at 11:05 AM Will Deacon <will@kernel.org> wrote:
> >
> > On Thu, Jan 09, 2020 at 08:32:54AM +0000, Will Deacon wrote:
> > > On Thu, Jan 09, 2020 at 09:46:55AM +0900, AKASHI Takahiro wrote:
> > > > On Wed, Jan 08, 2020 at 05:48:39PM +0000, Will Deacon wrote:
> > > > > On Mon, Dec 16, 2019 at 11:12:47AM +0900, AKASHI Takahiro wrote:
> > > > > > diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
> > > > > > index 12a561a54128..d24b527e8c00 100644
> > > > > > --- a/arch/arm64/include/asm/kexec.h
> > > > > > +++ b/arch/arm64/include/asm/kexec.h
> > > > > > @@ -96,6 +96,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;
> > > > > >  };
> > > > >
> > > > > This conflicts with the cleanup work from Pavel. Please can you check my
> > > > > resolution? [1]
> > > >
> > > > I don't know why we need to change a type of dtb_mem,
> > > > otherwise it looks good.
> > > >
> > > > (I also assume that you notice that kimage_arch is of no use for kexec.)
> > >
> > > Yes, that's why I'd like the resolution checked. If you reckon it's cleaner
> > > to drop Pavel's patch altogether in light of your changes, we can do that
> > > instead.
> > >
> > > Thoughts?
> >
> > Well, I've reverted the cleanup patch so please shout if you'd prefer
> > something else.
> 
> As I understand, the only concern was the type change for dtb_mem.
> This was one of the review comments for my patch
> https://lore.kernel.org/lkml/20191204155938.2279686-21-pasha.tatashin@soleen.com/
> 
> (I believe it was from Marc Zyngier), I add a number of new fields,
> and they all should be phys_addr_t, this is why I change dtb_mem to
> phys_addr_t to be consistent.

Sure, but I've only queued the first part of your series and that cleanup
patch doesn't make a lot of sense when applied against Akashi's work. I'm
happy to take stuff on top if you both agree to it, but having half of the
struct use unsigned long and the other half use phys_addr_t is messy.

Will

Re: [PATCH v4 2/2] arm64: kexec_file: add crash dump support

[AKASHI Takahiro]

Will, Pavel,

On Mon, Jan 13, 2020 at 11:21:06AM +0000, Will Deacon wrote:
> On Fri, Jan 10, 2020 at 11:19:16AM -0500, Pavel Tatashin wrote:
> > On Fri, Jan 10, 2020 at 11:05 AM Will Deacon <will@kernel.org> wrote:
> > >
> > > On Thu, Jan 09, 2020 at 08:32:54AM +0000, Will Deacon wrote:
> > > > On Thu, Jan 09, 2020 at 09:46:55AM +0900, AKASHI Takahiro wrote:
> > > > > On Wed, Jan 08, 2020 at 05:48:39PM +0000, Will Deacon wrote:
> > > > > > On Mon, Dec 16, 2019 at 11:12:47AM +0900, AKASHI Takahiro wrote:
> > > > > > > diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
> > > > > > > index 12a561a54128..d24b527e8c00 100644
> > > > > > > --- a/arch/arm64/include/asm/kexec.h
> > > > > > > +++ b/arch/arm64/include/asm/kexec.h
> > > > > > > @@ -96,6 +96,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;
> > > > > > >  };
> > > > > >
> > > > > > This conflicts with the cleanup work from Pavel. Please can you check my
> > > > > > resolution? [1]
> > > > >
> > > > > I don't know why we need to change a type of dtb_mem,
> > > > > otherwise it looks good.
> > > > >
> > > > > (I also assume that you notice that kimage_arch is of no use for kexec.)
> > > >
> > > > Yes, that's why I'd like the resolution checked. If you reckon it's cleaner
> > > > to drop Pavel's patch altogether in light of your changes, we can do that
> > > > instead.
> > > >
> > > > Thoughts?
> > >
> > > Well, I've reverted the cleanup patch so please shout if you'd prefer
> > > something else.
> > 
> > As I understand, the only concern was the type change for dtb_mem.
> > This was one of the review comments for my patch
> > https://lore.kernel.org/lkml/20191204155938.2279686-21-pasha.tatashin@soleen.com/
> > 
> > (I believe it was from Marc Zyngier), I add a number of new fields,
> > and they all should be phys_addr_t, this is why I change dtb_mem to
> > phys_addr_t to be consistent.
> 
> Sure, but I've only queued the first part of your series and that cleanup
> patch doesn't make a lot of sense when applied against Akashi's work. I'm
> happy to take stuff on top if you both agree to it, but having half of the
> struct use unsigned long and the other half use phys_addr_t is messy.

Logically, whether dtb_mem is a "unsigned long" or phys_addr_t doesn't
matter unless the kernel is compiled under LLP64.
As far as the existing kexec code, either generic or arm64-specific,
is concerned, however, "unsigned long is widely used as a physical address
(For example, see kexec_buf definition) over the code.

(Oops, reboot_code_buffer_phys is a phys_addr_t :)

So as long as my kexec_file (and associated kdump) patch comes first
before Pavel's, I'd like to keep using "unsigned long".
Then, you can change "unsigned long" to phys_addr_t in your patch
for whatever reason it is.

Please note that, if you want to do that, it would be better to modify
not only kimage_arch but also all the occurrences of "unsigned long"
to phys_addr_t for maintaining the integrity.

In addition, in my kexec_file kdump code, I still believe that
"#ifdef CONFIG_KEXEC_FILE" should stay before the definition of
kimage_arch as kimage_arch is of no use for normal kexec code.

Again,
"#ifdef" statement may be moved forward once additional fields be
added later by Pavel's patch, say, "[PATCH v8 15/25] arm64: kexec:
move relocation function setup" for any reason.

I believe that this way gives us a logical and consistent view of
history of changes.
Make sense?

Thanks,
-Takahiro Akashi


> Will

Re: [PATCH v4 2/2] arm64: kexec_file: add crash dump support

[Will Deacon]

On Tue, Jan 14, 2020 at 02:38:26PM +0900, AKASHI Takahiro wrote:
> On Mon, Jan 13, 2020 at 11:21:06AM +0000, Will Deacon wrote:
> > On Fri, Jan 10, 2020 at 11:19:16AM -0500, Pavel Tatashin wrote:
> > > On Fri, Jan 10, 2020 at 11:05 AM Will Deacon <will@kernel.org> wrote:
> > > > On Thu, Jan 09, 2020 at 08:32:54AM +0000, Will Deacon wrote:
> > > > > On Thu, Jan 09, 2020 at 09:46:55AM +0900, AKASHI Takahiro wrote:
> > > > > > On Wed, Jan 08, 2020 at 05:48:39PM +0000, Will Deacon wrote:
> > > > > > > On Mon, Dec 16, 2019 at 11:12:47AM +0900, AKASHI Takahiro wrote:
> > > > > > > > diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
> > > > > > > > index 12a561a54128..d24b527e8c00 100644
> > > > > > > > --- a/arch/arm64/include/asm/kexec.h
> > > > > > > > +++ b/arch/arm64/include/asm/kexec.h
> > > > > > > > @@ -96,6 +96,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;
> > > > > > > >  };
> > > > > > >
> > > > > > > This conflicts with the cleanup work from Pavel. Please can you check my
> > > > > > > resolution? [1]
> > > > > >
> > > > > > I don't know why we need to change a type of dtb_mem,
> > > > > > otherwise it looks good.
> > > > > >
> > > > > > (I also assume that you notice that kimage_arch is of no use for kexec.)
> > > > >
> > > > > Yes, that's why I'd like the resolution checked. If you reckon it's cleaner
> > > > > to drop Pavel's patch altogether in light of your changes, we can do that
> > > > > instead.
> > > > >
> > > > > Thoughts?
> > > >
> > > > Well, I've reverted the cleanup patch so please shout if you'd prefer
> > > > something else.
> > > 
> > > As I understand, the only concern was the type change for dtb_mem.
> > > This was one of the review comments for my patch
> > > https://lore.kernel.org/lkml/20191204155938.2279686-21-pasha.tatashin@soleen.com/
> > > 
> > > (I believe it was from Marc Zyngier), I add a number of new fields,
> > > and they all should be phys_addr_t, this is why I change dtb_mem to
> > > phys_addr_t to be consistent.
> > 
> > Sure, but I've only queued the first part of your series and that cleanup
> > patch doesn't make a lot of sense when applied against Akashi's work. I'm
> > happy to take stuff on top if you both agree to it, but having half of the
> > struct use unsigned long and the other half use phys_addr_t is messy.
> 
> Logically, whether dtb_mem is a "unsigned long" or phys_addr_t doesn't
> matter unless the kernel is compiled under LLP64.
> As far as the existing kexec code, either generic or arm64-specific,
> is concerned, however, "unsigned long is widely used as a physical address
> (For example, see kexec_buf definition) over the code.
> 
> (Oops, reboot_code_buffer_phys is a phys_addr_t :)
> 
> So as long as my kexec_file (and associated kdump) patch comes first
> before Pavel's, I'd like to keep using "unsigned long".
> Then, you can change "unsigned long" to phys_addr_t in your patch
> for whatever reason it is.
> 
> Please note that, if you want to do that, it would be better to modify
> not only kimage_arch but also all the occurrences of "unsigned long"
> to phys_addr_t for maintaining the integrity.
> 
> In addition, in my kexec_file kdump code, I still believe that
> "#ifdef CONFIG_KEXEC_FILE" should stay before the definition of
> kimage_arch as kimage_arch is of no use for normal kexec code.
> 
> Again,
> "#ifdef" statement may be moved forward once additional fields be
> added later by Pavel's patch, say, "[PATCH v8 15/25] arm64: kexec:
> move relocation function setup" for any reason.
> 
> I believe that this way gives us a logical and consistent view of
> history of changes.
> Make sense?

This is a bit much to stick in a merge commit, so I'll stick with the revert
for now and you can send patches on top if you want it changed.

Cheers,

Will

Re: [PATCH v4 2/2] arm64: kexec_file: add crash dump support

[AKASHI Takahiro]

On Thu, Jan 16, 2020 at 06:08:58PM +0000, Will Deacon wrote:
> On Tue, Jan 14, 2020 at 02:38:26PM +0900, AKASHI Takahiro wrote:
> > On Mon, Jan 13, 2020 at 11:21:06AM +0000, Will Deacon wrote:
> > > On Fri, Jan 10, 2020 at 11:19:16AM -0500, Pavel Tatashin wrote:
> > > > On Fri, Jan 10, 2020 at 11:05 AM Will Deacon <will@kernel.org> wrote:
> > > > > On Thu, Jan 09, 2020 at 08:32:54AM +0000, Will Deacon wrote:
> > > > > > On Thu, Jan 09, 2020 at 09:46:55AM +0900, AKASHI Takahiro wrote:
> > > > > > > On Wed, Jan 08, 2020 at 05:48:39PM +0000, Will Deacon wrote:
> > > > > > > > On Mon, Dec 16, 2019 at 11:12:47AM +0900, AKASHI Takahiro wrote:
> > > > > > > > > diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
> > > > > > > > > index 12a561a54128..d24b527e8c00 100644
> > > > > > > > > --- a/arch/arm64/include/asm/kexec.h
> > > > > > > > > +++ b/arch/arm64/include/asm/kexec.h
> > > > > > > > > @@ -96,6 +96,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;
> > > > > > > > >  };
> > > > > > > >
> > > > > > > > This conflicts with the cleanup work from Pavel. Please can you check my
> > > > > > > > resolution? [1]
> > > > > > >
> > > > > > > I don't know why we need to change a type of dtb_mem,
> > > > > > > otherwise it looks good.
> > > > > > >
> > > > > > > (I also assume that you notice that kimage_arch is of no use for kexec.)
> > > > > >
> > > > > > Yes, that's why I'd like the resolution checked. If you reckon it's cleaner
> > > > > > to drop Pavel's patch altogether in light of your changes, we can do that
> > > > > > instead.
> > > > > >
> > > > > > Thoughts?
> > > > >
> > > > > Well, I've reverted the cleanup patch so please shout if you'd prefer
> > > > > something else.
> > > > 
> > > > As I understand, the only concern was the type change for dtb_mem.
> > > > This was one of the review comments for my patch
> > > > https://lore.kernel.org/lkml/20191204155938.2279686-21-pasha.tatashin@soleen.com/
> > > > 
> > > > (I believe it was from Marc Zyngier), I add a number of new fields,
> > > > and they all should be phys_addr_t, this is why I change dtb_mem to
> > > > phys_addr_t to be consistent.
> > > 
> > > Sure, but I've only queued the first part of your series and that cleanup
> > > patch doesn't make a lot of sense when applied against Akashi's work. I'm
> > > happy to take stuff on top if you both agree to it, but having half of the
> > > struct use unsigned long and the other half use phys_addr_t is messy.
> > 
> > Logically, whether dtb_mem is a "unsigned long" or phys_addr_t doesn't
> > matter unless the kernel is compiled under LLP64.
> > As far as the existing kexec code, either generic or arm64-specific,
> > is concerned, however, "unsigned long is widely used as a physical address
> > (For example, see kexec_buf definition) over the code.
> > 
> > (Oops, reboot_code_buffer_phys is a phys_addr_t :)
> > 
> > So as long as my kexec_file (and associated kdump) patch comes first
> > before Pavel's, I'd like to keep using "unsigned long".
> > Then, you can change "unsigned long" to phys_addr_t in your patch
> > for whatever reason it is.
> > 
> > Please note that, if you want to do that, it would be better to modify
> > not only kimage_arch but also all the occurrences of "unsigned long"
> > to phys_addr_t for maintaining the integrity.
> > 
> > In addition, in my kexec_file kdump code, I still believe that
> > "#ifdef CONFIG_KEXEC_FILE" should stay before the definition of
> > kimage_arch as kimage_arch is of no use for normal kexec code.
> > 
> > Again,
> > "#ifdef" statement may be moved forward once additional fields be
> > added later by Pavel's patch, say, "[PATCH v8 15/25] arm64: kexec:
> > move relocation function setup" for any reason.
> > 
> > I believe that this way gives us a logical and consistent view of
> > history of changes.
> > Make sense?
> 
> This is a bit much to stick in a merge commit, so I'll stick with the revert
> for now and you can send patches on top if you want it changed.

Are you asking me or Pavel? And on top of which branch?

Thanks,
-Takahiro Akashi


> Cheers,
> 
> Will

Re: [PATCH v4 2/2] arm64: kexec_file: add crash dump support

[Will Deacon]

On Fri, Jan 17, 2020 at 03:38:33PM +0900, AKASHI Takahiro wrote:
> On Thu, Jan 16, 2020 at 06:08:58PM +0000, Will Deacon wrote:
> > On Tue, Jan 14, 2020 at 02:38:26PM +0900, AKASHI Takahiro wrote:
> > > On Mon, Jan 13, 2020 at 11:21:06AM +0000, Will Deacon wrote:
> > > > On Fri, Jan 10, 2020 at 11:19:16AM -0500, Pavel Tatashin wrote:
> > > > > On Fri, Jan 10, 2020 at 11:05 AM Will Deacon <will@kernel.org> wrote:
> > > > > > On Thu, Jan 09, 2020 at 08:32:54AM +0000, Will Deacon wrote:
> > > > > > > On Thu, Jan 09, 2020 at 09:46:55AM +0900, AKASHI Takahiro wrote:
> > > > > > > > On Wed, Jan 08, 2020 at 05:48:39PM +0000, Will Deacon wrote:
> > > > > > > > > On Mon, Dec 16, 2019 at 11:12:47AM +0900, AKASHI Takahiro wrote:
> > > > > > > > > > diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
> > > > > > > > > > index 12a561a54128..d24b527e8c00 100644
> > > > > > > > > > --- a/arch/arm64/include/asm/kexec.h
> > > > > > > > > > +++ b/arch/arm64/include/asm/kexec.h
> > > > > > > > > > @@ -96,6 +96,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;
> > > > > > > > > >  };
> > > > > > > > >
> > > > > > > > > This conflicts with the cleanup work from Pavel. Please can you check my
> > > > > > > > > resolution? [1]
> > > > > > > >
> > > > > > > > I don't know why we need to change a type of dtb_mem,
> > > > > > > > otherwise it looks good.
> > > > > > > >
> > > > > > > > (I also assume that you notice that kimage_arch is of no use for kexec.)
> > > > > > >
> > > > > > > Yes, that's why I'd like the resolution checked. If you reckon it's cleaner
> > > > > > > to drop Pavel's patch altogether in light of your changes, we can do that
> > > > > > > instead.
> > > > > > >
> > > > > > > Thoughts?
> > > > > >
> > > > > > Well, I've reverted the cleanup patch so please shout if you'd prefer
> > > > > > something else.
> > > > > 
> > > > > As I understand, the only concern was the type change for dtb_mem.
> > > > > This was one of the review comments for my patch
> > > > > https://lore.kernel.org/lkml/20191204155938.2279686-21-pasha.tatashin@soleen.com/
> > > > > 
> > > > > (I believe it was from Marc Zyngier), I add a number of new fields,
> > > > > and they all should be phys_addr_t, this is why I change dtb_mem to
> > > > > phys_addr_t to be consistent.
> > > > 
> > > > Sure, but I've only queued the first part of your series and that cleanup
> > > > patch doesn't make a lot of sense when applied against Akashi's work. I'm
> > > > happy to take stuff on top if you both agree to it, but having half of the
> > > > struct use unsigned long and the other half use phys_addr_t is messy.
> > > 
> > > Logically, whether dtb_mem is a "unsigned long" or phys_addr_t doesn't
> > > matter unless the kernel is compiled under LLP64.
> > > As far as the existing kexec code, either generic or arm64-specific,
> > > is concerned, however, "unsigned long is widely used as a physical address
> > > (For example, see kexec_buf definition) over the code.
> > > 
> > > (Oops, reboot_code_buffer_phys is a phys_addr_t :)
> > > 
> > > So as long as my kexec_file (and associated kdump) patch comes first
> > > before Pavel's, I'd like to keep using "unsigned long".
> > > Then, you can change "unsigned long" to phys_addr_t in your patch
> > > for whatever reason it is.
> > > 
> > > Please note that, if you want to do that, it would be better to modify
> > > not only kimage_arch but also all the occurrences of "unsigned long"
> > > to phys_addr_t for maintaining the integrity.
> > > 
> > > In addition, in my kexec_file kdump code, I still believe that
> > > "#ifdef CONFIG_KEXEC_FILE" should stay before the definition of
> > > kimage_arch as kimage_arch is of no use for normal kexec code.
> > > 
> > > Again,
> > > "#ifdef" statement may be moved forward once additional fields be
> > > added later by Pavel's patch, say, "[PATCH v8 15/25] arm64: kexec:
> > > move relocation function setup" for any reason.
> > > 
> > > I believe that this way gives us a logical and consistent view of
> > > history of changes.
> > > Make sense?
> > 
> > This is a bit much to stick in a merge commit, so I'll stick with the revert
> > for now and you can send patches on top if you want it changed.
> 
> Are you asking me or Pavel? And on top of which branch?

I'm not asking anything ;p

But by "you" I mean both of you (the joys of ambiguous English). In other
words, I've reverted the patch [1], but I'm happy to take other patches on
top providing that you both agree with each other on what you want to do.

Will

[1] https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/commit/?h=for-next/kexec/cleanup&id=1595fe299eb5a664c754eaf48bc178c0d664e1cf