* [PATCH v4 0/2] arm64: kexec_file: add kdump
@ 2019-12-16 2:12 AKASHI Takahiro
2019-12-16 2:12 ` [PATCH v4 1/2] libfdt: include fdt_addresses.c AKASHI Takahiro
2019-12-16 2:12 ` [PATCH v4 2/2] arm64: kexec_file: add crash dump support AKASHI Takahiro
0 siblings, 2 replies; 14+ messages in thread
From: AKASHI Takahiro @ 2019-12-16 2:12 UTC (permalink / raw)
To: catalin.marinas, will.deacon, robh+dt, frowand.list
Cc: james.morse, bhsharma, kexec, linux-arm-kernel, linux-kernel,
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
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH v4 1/2] libfdt: include fdt_addresses.c 2019-12-16 2:12 [PATCH v4 0/2] arm64: kexec_file: add kdump AKASHI Takahiro @ 2019-12-16 2:12 ` AKASHI Takahiro 2019-12-16 2:12 ` [PATCH v4 2/2] arm64: kexec_file: add crash dump support AKASHI Takahiro 1 sibling, 0 replies; 14+ messages in thread From: AKASHI Takahiro @ 2019-12-16 2:12 UTC (permalink / raw) To: catalin.marinas, will.deacon, robh+dt, frowand.list Cc: james.morse, bhsharma, kexec, linux-arm-kernel, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v4 2/2] arm64: kexec_file: add crash dump support 2019-12-16 2:12 [PATCH v4 0/2] arm64: kexec_file: add kdump AKASHI Takahiro 2019-12-16 2:12 ` [PATCH v4 1/2] libfdt: include fdt_addresses.c AKASHI Takahiro @ 2019-12-16 2:12 ` AKASHI Takahiro 2020-01-08 17:48 ` Will Deacon 1 sibling, 1 reply; 14+ messages in thread From: AKASHI Takahiro @ 2019-12-16 2:12 UTC (permalink / raw) To: catalin.marinas, will.deacon, robh+dt, frowand.list Cc: james.morse, bhsharma, kexec, linux-arm-kernel, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] arm64: kexec_file: add crash dump support 2019-12-16 2:12 ` [PATCH v4 2/2] arm64: kexec_file: add crash dump support AKASHI Takahiro @ 2020-01-08 17:48 ` Will Deacon 2020-01-08 17:58 ` Pavel Tatashin 2020-01-09 0:46 ` AKASHI Takahiro 0 siblings, 2 replies; 14+ messages in thread From: Will Deacon @ 2020-01-08 17:48 UTC (permalink / raw) To: AKASHI Takahiro, pasha.tatashin Cc: catalin.marinas, will.deacon, robh+dt, frowand.list, bhsharma, kexec, linux-kernel, james.morse, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] arm64: kexec_file: add crash dump support 2020-01-08 17:48 ` Will Deacon @ 2020-01-08 17:58 ` Pavel Tatashin 2020-01-09 0:46 ` AKASHI Takahiro 1 sibling, 0 replies; 14+ messages in thread From: Pavel Tatashin @ 2020-01-08 17:58 UTC (permalink / raw) To: Will Deacon Cc: AKASHI Takahiro, Catalin Marinas, Will Deacon, robh+dt, frowand.list, Bhupesh Sharma, kexec mailing list, LKML, James Morse, Linux ARM 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] arm64: kexec_file: add crash dump support 2020-01-08 17:48 ` Will Deacon 2020-01-08 17:58 ` Pavel Tatashin @ 2020-01-09 0:46 ` AKASHI Takahiro 2020-01-09 8:32 ` Will Deacon 1 sibling, 1 reply; 14+ messages in thread From: AKASHI Takahiro @ 2020-01-09 0:46 UTC (permalink / raw) To: Will Deacon Cc: pasha.tatashin, catalin.marinas, will.deacon, robh+dt, frowand.list, bhsharma, kexec, linux-kernel, james.morse, linux-arm-kernel 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] arm64: kexec_file: add crash dump support 2020-01-09 0:46 ` AKASHI Takahiro @ 2020-01-09 8:32 ` Will Deacon 2020-01-10 16:05 ` Will Deacon 0 siblings, 1 reply; 14+ messages in thread From: Will Deacon @ 2020-01-09 8:32 UTC (permalink / raw) To: AKASHI Takahiro, pasha.tatashin, catalin.marinas, will.deacon, robh+dt, frowand.list, bhsharma, kexec, linux-kernel, james.morse, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] arm64: kexec_file: add crash dump support 2020-01-09 8:32 ` Will Deacon @ 2020-01-10 16:05 ` Will Deacon 2020-01-10 16:19 ` Pavel Tatashin 0 siblings, 1 reply; 14+ messages in thread From: Will Deacon @ 2020-01-10 16:05 UTC (permalink / raw) To: AKASHI Takahiro, pasha.tatashin, catalin.marinas, will.deacon, robh+dt, frowand.list, bhsharma, kexec, linux-kernel, james.morse, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] arm64: kexec_file: add crash dump support 2020-01-10 16:05 ` Will Deacon @ 2020-01-10 16:19 ` Pavel Tatashin 2020-01-13 11:21 ` Will Deacon 0 siblings, 1 reply; 14+ messages in thread From: Pavel Tatashin @ 2020-01-10 16:19 UTC (permalink / raw) To: Will Deacon Cc: AKASHI Takahiro, Catalin Marinas, Will Deacon, robh+dt, frowand.list, Bhupesh Sharma, kexec mailing list, LKML, James Morse, Linux ARM 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] arm64: kexec_file: add crash dump support 2020-01-10 16:19 ` Pavel Tatashin @ 2020-01-13 11:21 ` Will Deacon 2020-01-14 5:38 ` AKASHI Takahiro 0 siblings, 1 reply; 14+ messages in thread From: Will Deacon @ 2020-01-13 11:21 UTC (permalink / raw) To: Pavel Tatashin Cc: AKASHI Takahiro, Catalin Marinas, Will Deacon, robh+dt, frowand.list, Bhupesh Sharma, kexec mailing list, LKML, James Morse, Linux ARM 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] arm64: kexec_file: add crash dump support 2020-01-13 11:21 ` Will Deacon @ 2020-01-14 5:38 ` AKASHI Takahiro 2020-01-16 18:08 ` Will Deacon 0 siblings, 1 reply; 14+ messages in thread From: AKASHI Takahiro @ 2020-01-14 5:38 UTC (permalink / raw) To: Will Deacon Cc: Pavel Tatashin, Catalin Marinas, Will Deacon, robh+dt, frowand.list, Bhupesh Sharma, kexec mailing list, LKML, James Morse, Linux ARM 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] arm64: kexec_file: add crash dump support 2020-01-14 5:38 ` AKASHI Takahiro @ 2020-01-16 18:08 ` Will Deacon 2020-01-17 6:38 ` AKASHI Takahiro 0 siblings, 1 reply; 14+ messages in thread From: Will Deacon @ 2020-01-16 18:08 UTC (permalink / raw) To: AKASHI Takahiro, Pavel Tatashin, Catalin Marinas, Will Deacon, robh+dt, frowand.list, Bhupesh Sharma, kexec mailing list, LKML, James Morse, Linux ARM 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] arm64: kexec_file: add crash dump support 2020-01-16 18:08 ` Will Deacon @ 2020-01-17 6:38 ` AKASHI Takahiro 2020-01-17 12:45 ` Will Deacon 0 siblings, 1 reply; 14+ messages in thread From: AKASHI Takahiro @ 2020-01-17 6:38 UTC (permalink / raw) To: Will Deacon Cc: Pavel Tatashin, Catalin Marinas, Will Deacon, robh+dt, frowand.list, Bhupesh Sharma, kexec mailing list, LKML, James Morse, Linux ARM 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] arm64: kexec_file: add crash dump support 2020-01-17 6:38 ` AKASHI Takahiro @ 2020-01-17 12:45 ` Will Deacon 0 siblings, 0 replies; 14+ messages in thread From: Will Deacon @ 2020-01-17 12:45 UTC (permalink / raw) To: AKASHI Takahiro, Pavel Tatashin, Catalin Marinas, Will Deacon, robh+dt, frowand.list, Bhupesh Sharma, kexec mailing list, LKML, James Morse, Linux ARM 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-01-17 12:45 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-16 2:12 [PATCH v4 0/2] arm64: kexec_file: add kdump AKASHI Takahiro 2019-12-16 2:12 ` [PATCH v4 1/2] libfdt: include fdt_addresses.c AKASHI Takahiro 2019-12-16 2:12 ` [PATCH v4 2/2] arm64: kexec_file: add crash dump support AKASHI Takahiro 2020-01-08 17:48 ` Will Deacon 2020-01-08 17:58 ` Pavel Tatashin 2020-01-09 0:46 ` AKASHI Takahiro 2020-01-09 8:32 ` Will Deacon 2020-01-10 16:05 ` Will Deacon 2020-01-10 16:19 ` Pavel Tatashin 2020-01-13 11:21 ` Will Deacon 2020-01-14 5:38 ` AKASHI Takahiro 2020-01-16 18:08 ` Will Deacon 2020-01-17 6:38 ` AKASHI Takahiro 2020-01-17 12:45 ` Will Deacon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).