| Message ID | 20191216021247.24950-3-takahiro.akashi@linaro.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 3751e728cef2908c15974a5ae44627fd41ef3362 |
| Headers | show |
| Series | arm64: kexec_file: add kdump | expand |
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
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 >
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 >
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
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
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
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
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
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
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
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
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;