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 <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Reviewed-by: James Morse <[email protected]>
> Tested-and-reviewed-by: Bhupesh Sharma <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]>
> > Cc: Catalin Marinas <[email protected]>
> > Cc: Will Deacon <[email protected]>
> > Reviewed-by: James Morse <[email protected]>
> > Tested-and-reviewed-by: Bhupesh Sharma <[email protected]>
> > ---
> > 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 <[email protected]>
> > Cc: Catalin Marinas <[email protected]>
> > Cc: Will Deacon <[email protected]>
> > Reviewed-by: James Morse <[email protected]>
> > Tested-and-reviewed-by: Bhupesh Sharma <[email protected]>
> > ---
> > 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 <[email protected]>
> > > Cc: Catalin Marinas <[email protected]>
> > > Cc: Will Deacon <[email protected]>
> > > Reviewed-by: James Morse <[email protected]>
> > > Tested-and-reviewed-by: Bhupesh Sharma <[email protected]>
> > > ---
> > > 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 <[email protected]> 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/[email protected]/
(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 <[email protected]> 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/[email protected]/
>
> (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 <[email protected]> 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/[email protected]/
> >
> > (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 <[email protected]> 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/[email protected]/
> > >
> > > (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 <[email protected]> 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/[email protected]/
> > > >
> > > > (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 <[email protected]> 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/[email protected]/
> > > > >
> > > > > (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