| Message ID | 20180425062629.29404-4-takahiro.akashi@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Akashi, On 25/04/18 07:26, AKASHI Takahiro wrote: > On arm64, purugatory would do almosty nothing. So just invoke secondary > kernel directy by jumping into its entry code. (Nits: purgatory, almost, directly) > While, in this case, cpu_soft_restart() must be called with dtb address > in the fifth argument, the behavior still stays compatible with kexec_load > case as long as the argument is null. > diff --git a/arch/arm64/kernel/cpu-reset.S b/arch/arm64/kernel/cpu-reset.S > index 8021b46c9743..391df91328ac 100644 > --- a/arch/arm64/kernel/cpu-reset.S > +++ b/arch/arm64/kernel/cpu-reset.S > @@ -24,9 +24,9 @@ > * > * @el2_switch: Flag to indicate a swich to EL2 is needed. (Nit: switch) > * @entry: Location to jump to for soft reset. > - * arg0: First argument passed to @entry. > - * arg1: Second argument passed to @entry. > - * arg2: Third argument passed to @entry. > + * arg0: First argument passed to @entry. (relocation list) > + * arg1: Second argument passed to @entry.(physcal kernel entry) (Nit: physical) > + * arg2: Third argument passed to @entry. (physical dtb address) > * > * Put the CPU into the same state as it would be if it had been reset, and > * branch to what would be the reset vector. It must be executed with the > diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c > index f76ea92dff91..f7dbba00be10 100644 > --- a/arch/arm64/kernel/machine_kexec.c > +++ b/arch/arm64/kernel/machine_kexec.c > @@ -205,10 +205,17 @@ void machine_kexec(struct kimage *kimage) > * uses physical addressing to relocate the new image to its final > * position and transfers control to the image entry point when the > * relocation is complete. > + * In case of kexec_file_load syscall, we directly start the kernel, > + * skipping purgatory. We're not really skipping purgatory, purgatory doesn't exist! For regular kexec the image/payload we run is up to kexec-tools. For kexec_file_load its a kernel-image. Purgatory is a kexec-tools-ism. > cpu_soft_restart(kimage != kexec_crash_image, > - reboot_code_buffer_phys, kimage->head, kimage->start, 0); > + reboot_code_buffer_phys, kimage->head, kimage->start, > +#ifdef CONFIG_KEXEC_FILE > + kimage->purgatory_info.purgatory_buf ? > + 0 : kimage->arch.dtb_mem); > +#else > + 0); > +#endif Where does kimage->arch.dtb_mem come from? This patch won't build until patch 8 adds the config option, which is going to make bisecting any kexec side-effects tricky. purgatory_buf seems to only be set in kexec_purgatory_setup_kbuf(), called from kexec_load_purgatory(), which we don't use. How does this get a value? Would it be better to always use kimage->arch.dtb_mem, and ensure that is 0 for regular kexec (as we can't know where the dtb is)? (image_arg may then be a better name). Thanks, James
On Tue, May 01, 2018 at 06:46:06PM +0100, James Morse wrote: > Hi Akashi, > > On 25/04/18 07:26, AKASHI Takahiro wrote: > > On arm64, purugatory would do almosty nothing. So just invoke secondary > > kernel directy by jumping into its entry code. > > (Nits: purgatory, almost, directly) Oops, I think I ran spell before ... > > > While, in this case, cpu_soft_restart() must be called with dtb address > > in the fifth argument, the behavior still stays compatible with kexec_load > > case as long as the argument is null. > > > > diff --git a/arch/arm64/kernel/cpu-reset.S b/arch/arm64/kernel/cpu-reset.S > > index 8021b46c9743..391df91328ac 100644 > > --- a/arch/arm64/kernel/cpu-reset.S > > +++ b/arch/arm64/kernel/cpu-reset.S > > @@ -24,9 +24,9 @@ > > * > > * @el2_switch: Flag to indicate a swich to EL2 is needed. > > (Nit: switch) ditto > > * @entry: Location to jump to for soft reset. > > - * arg0: First argument passed to @entry. > > - * arg1: Second argument passed to @entry. > > - * arg2: Third argument passed to @entry. > > + * arg0: First argument passed to @entry. (relocation list) > > + * arg1: Second argument passed to @entry.(physcal kernel entry) > > (Nit: physical) ditto > > > + * arg2: Third argument passed to @entry. (physical dtb address) > > * > > * Put the CPU into the same state as it would be if it had been reset, and > > * branch to what would be the reset vector. It must be executed with the > > diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c > > index f76ea92dff91..f7dbba00be10 100644 > > --- a/arch/arm64/kernel/machine_kexec.c > > +++ b/arch/arm64/kernel/machine_kexec.c > > @@ -205,10 +205,17 @@ void machine_kexec(struct kimage *kimage) > > * uses physical addressing to relocate the new image to its final > > * position and transfers control to the image entry point when the > > * relocation is complete. > > + * In case of kexec_file_load syscall, we directly start the kernel, > > + * skipping purgatory. > > We're not really skipping purgatory, purgatory doesn't exist! For regular kexec > the image/payload we run is up to kexec-tools. For kexec_file_load its a > kernel-image. Purgatory is a kexec-tools-ism. You are right, but in general, purgatory is expected to exist by generic kexec code and does exist on all architectures, kexec_load() or kexec_file_load(), except arm64's kexec_file_load case. So it would be nice to have some explicit notes here. > > > cpu_soft_restart(kimage != kexec_crash_image, > > - reboot_code_buffer_phys, kimage->head, kimage->start, 0); > > + reboot_code_buffer_phys, kimage->head, kimage->start, > > +#ifdef CONFIG_KEXEC_FILE > > + kimage->purgatory_info.purgatory_buf ? > > + 0 : kimage->arch.dtb_mem); > > +#else > > + 0); > > +#endif > > Where does kimage->arch.dtb_mem come from? This patch won't build until patch 8 > adds the config option, which is going to make bisecting any kexec side-effects > tricky. CONFIG_KEXEC_FILE is also used in patch #4, #5 and #6. I don't know how we can fix this as the implementation is divided into several patches. (So bisecting doesn't work anyway.) > purgatory_buf seems to only be set in kexec_purgatory_setup_kbuf(), called from > kexec_load_purgatory(), which we don't use. How does this get a value? > > Would it be better to always use kimage->arch.dtb_mem, and ensure that is 0 for > regular kexec (as we can't know where the dtb is)? (image_arg may then be a > better name). The problem is arch.dtb_mem is currently defined only if CONFIG_KEXEC_FILE. So I would like to - merge this patch with patch#8 - change the condition #ifdef CONFIG_KEXEC_FILE kimage->file_mode ? kimage->arch.dtb_mem : 0); #else 0); #endif Thanks, -Takahiro AKASHI > > Thanks, > > James
Hi Akashi, On 07/05/18 06:22, AKASHI Takahiro wrote: > On Tue, May 01, 2018 at 06:46:06PM +0100, James Morse wrote: >> On 25/04/18 07:26, AKASHI Takahiro wrote: >>> diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c >>> index f76ea92dff91..f7dbba00be10 100644 >>> --- a/arch/arm64/kernel/machine_kexec.c >>> +++ b/arch/arm64/kernel/machine_kexec.c >>> @@ -205,10 +205,17 @@ void machine_kexec(struct kimage *kimage) >>> cpu_soft_restart(kimage != kexec_crash_image, >>> - reboot_code_buffer_phys, kimage->head, kimage->start, 0); >>> + reboot_code_buffer_phys, kimage->head, kimage->start, >>> +#ifdef CONFIG_KEXEC_FILE >>> + kimage->purgatory_info.purgatory_buf ? >>> + 0 : kimage->arch.dtb_mem); >>> +#else >>> + 0); >>> +#endif >> purgatory_buf seems to only be set in kexec_purgatory_setup_kbuf(), called from >> kexec_load_purgatory(), which we don't use. How does this get a value? >> >> Would it be better to always use kimage->arch.dtb_mem, and ensure that is 0 for >> regular kexec (as we can't know where the dtb is)? (image_arg may then be a >> better name). > > The problem is arch.dtb_mem is currently defined only if CONFIG_KEXEC_FILE. I thought it was ARCH_HAS_KIMAGE_ARCH, which we can define all the time if that's what we want. > So I would like to > - merge this patch with patch#8 > - change the condition > #ifdef CONFIG_KEXEC_FILE > kimage->file_mode ? kimage->arch.dtb_mem : 0); > #else > 0); > #endif If we can avoid even this #ifdef by always having kimage->arch, I'd prefer that. If we do that 'dtb_mem' would need some thing that indicates its for kexec_file, as kexec has a DTB too, we just don't know where it is... Thanks, James
James, On Fri, May 11, 2018 at 06:03:49PM +0100, James Morse wrote: > Hi Akashi, > > On 07/05/18 06:22, AKASHI Takahiro wrote: > > On Tue, May 01, 2018 at 06:46:06PM +0100, James Morse wrote: > >> On 25/04/18 07:26, AKASHI Takahiro wrote: > >>> diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c > >>> index f76ea92dff91..f7dbba00be10 100644 > >>> --- a/arch/arm64/kernel/machine_kexec.c > >>> +++ b/arch/arm64/kernel/machine_kexec.c > >>> @@ -205,10 +205,17 @@ void machine_kexec(struct kimage *kimage) > > >>> cpu_soft_restart(kimage != kexec_crash_image, > >>> - reboot_code_buffer_phys, kimage->head, kimage->start, 0); > >>> + reboot_code_buffer_phys, kimage->head, kimage->start, > >>> +#ifdef CONFIG_KEXEC_FILE > >>> + kimage->purgatory_info.purgatory_buf ? > >>> + 0 : kimage->arch.dtb_mem); > >>> +#else > >>> + 0); > >>> +#endif > > > >> purgatory_buf seems to only be set in kexec_purgatory_setup_kbuf(), called from > >> kexec_load_purgatory(), which we don't use. How does this get a value? > >> > >> Would it be better to always use kimage->arch.dtb_mem, and ensure that is 0 for > >> regular kexec (as we can't know where the dtb is)? (image_arg may then be a > >> better name). > > > > The problem is arch.dtb_mem is currently defined only if CONFIG_KEXEC_FILE. > > I thought it was ARCH_HAS_KIMAGE_ARCH, which we can define all the time if > that's what we want. > > > > So I would like to > > - merge this patch with patch#8 > > - change the condition > > #ifdef CONFIG_KEXEC_FILE > > kimage->file_mode ? kimage->arch.dtb_mem : 0); > > #else > > 0); > > #endif > > If we can avoid even this #ifdef by always having kimage->arch, I'd prefer that. > If we do that 'dtb_mem' would need some thing that indicates its for kexec_file, > as kexec has a DTB too, we just don't know where it is... OK, but I want to have a minimum of kexec.arch always exist. How about this? | #define ARCH_HAS_KIMAGE_ARCH | | struct kimage_arch { | phys_addr_t dtb_mem; | #ifdef CONFIG_KEXEC_FILE | void *dtb_buf; | /* Core ELF header buffer */ | void *elf_headers; | unsigned long elf_headers_sz; | unsigned long elf_load_addr; | #endif | void machine_kexec(struct kimage *kimage) | { | ... | cpu_soft_restart(kimage != kexec_crash_image, | reboot_code_buffer_phys, kimage->head, kimage->start, | kimage->arch.dtb_mem); Thanks -Takahiro AKASHI > > > Thanks, > > James
Hi Akashi, On 15/05/18 05:45, AKASHI Takahiro wrote: > On Fri, May 11, 2018 at 06:03:49PM +0100, James Morse wrote: >> On 07/05/18 06:22, AKASHI Takahiro wrote: >>> On Tue, May 01, 2018 at 06:46:06PM +0100, James Morse wrote: >>>> On 25/04/18 07:26, AKASHI Takahiro wrote: >>>>> diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c >>>>> index f76ea92dff91..f7dbba00be10 100644 >>>>> --- a/arch/arm64/kernel/machine_kexec.c >>>>> +++ b/arch/arm64/kernel/machine_kexec.c >>>>> @@ -205,10 +205,17 @@ void machine_kexec(struct kimage *kimage) >> >>>>> cpu_soft_restart(kimage != kexec_crash_image, >>>>> - reboot_code_buffer_phys, kimage->head, kimage->start, 0); >>>>> + reboot_code_buffer_phys, kimage->head, kimage->start, >>>>> +#ifdef CONFIG_KEXEC_FILE >>>>> + kimage->purgatory_info.purgatory_buf ? >>>>> + 0 : kimage->arch.dtb_mem); >>>>> +#else >>>>> + 0); >>>>> +#endif >> >> >>>> purgatory_buf seems to only be set in kexec_purgatory_setup_kbuf(), called from >>>> kexec_load_purgatory(), which we don't use. How does this get a value? >>>> >>>> Would it be better to always use kimage->arch.dtb_mem, and ensure that is 0 for >>>> regular kexec (as we can't know where the dtb is)? (image_arg may then be a >>>> better name). >>> >>> The problem is arch.dtb_mem is currently defined only if CONFIG_KEXEC_FILE. >> >> I thought it was ARCH_HAS_KIMAGE_ARCH, which we can define all the time if >> that's what we want. >> >> >>> So I would like to >>> - merge this patch with patch#8 >>> - change the condition >>> #ifdef CONFIG_KEXEC_FILE >>> kimage->file_mode ? kimage->arch.dtb_mem : 0); >>> #else >>> 0); >>> #endif >> >> If we can avoid even this #ifdef by always having kimage->arch, I'd prefer that. >> If we do that 'dtb_mem' would need some thing that indicates its for kexec_file, >> as kexec has a DTB too, we just don't know where it is... > > OK, but I want to have a minimum of kexec.arch always exist. I'm curious, why? Its 32bytes that is allocated a maximum of twice. (my questions on what needs to go in there were because it looked like a third user was missing...) > How about this? > > | struct kimage_arch { > | phys_addr_t dtb_mem; > | #ifdef CONFIG_KEXEC_FILE #ifdef in structs just breeds more #ifdefs, as the code that accesses those members has to be behind the same set of conditions. Given this, I prefer the #ifdefs around cpu_soft_restart() as it doesn't force us to add more #ifdefs later. For either option without purgatory_info: Reviewed-by: James Morse <james.morse@arm.com> Thanks, James
James, On Tue, May 15, 2018 at 05:15:52PM +0100, James Morse wrote: > Hi Akashi, > > On 15/05/18 05:45, AKASHI Takahiro wrote: > > On Fri, May 11, 2018 at 06:03:49PM +0100, James Morse wrote: > >> On 07/05/18 06:22, AKASHI Takahiro wrote: > >>> On Tue, May 01, 2018 at 06:46:06PM +0100, James Morse wrote: > >>>> On 25/04/18 07:26, AKASHI Takahiro wrote: > >>>>> diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c > >>>>> index f76ea92dff91..f7dbba00be10 100644 > >>>>> --- a/arch/arm64/kernel/machine_kexec.c > >>>>> +++ b/arch/arm64/kernel/machine_kexec.c > >>>>> @@ -205,10 +205,17 @@ void machine_kexec(struct kimage *kimage) > >> > >>>>> cpu_soft_restart(kimage != kexec_crash_image, > >>>>> - reboot_code_buffer_phys, kimage->head, kimage->start, 0); > >>>>> + reboot_code_buffer_phys, kimage->head, kimage->start, > >>>>> +#ifdef CONFIG_KEXEC_FILE > >>>>> + kimage->purgatory_info.purgatory_buf ? > >>>>> + 0 : kimage->arch.dtb_mem); > >>>>> +#else > >>>>> + 0); > >>>>> +#endif > >> > >> > >>>> purgatory_buf seems to only be set in kexec_purgatory_setup_kbuf(), called from > >>>> kexec_load_purgatory(), which we don't use. How does this get a value? > >>>> > >>>> Would it be better to always use kimage->arch.dtb_mem, and ensure that is 0 for > >>>> regular kexec (as we can't know where the dtb is)? (image_arg may then be a > >>>> better name). > >>> > >>> The problem is arch.dtb_mem is currently defined only if CONFIG_KEXEC_FILE. > >> > >> I thought it was ARCH_HAS_KIMAGE_ARCH, which we can define all the time if > >> that's what we want. > >> > >> > >>> So I would like to > >>> - merge this patch with patch#8 > >>> - change the condition > >>> #ifdef CONFIG_KEXEC_FILE > >>> kimage->file_mode ? kimage->arch.dtb_mem : 0); We don't need "kimage->file_mode ?" since arch.dtb_mem is 0 if !file_mode. > >>> #else > >>> 0); > >>> #endif > >> > >> If we can avoid even this #ifdef by always having kimage->arch, I'd prefer that. > >> If we do that 'dtb_mem' would need some thing that indicates its for kexec_file, > >> as kexec has a DTB too, we just don't know where it is... > > > > OK, but I want to have a minimum of kexec.arch always exist. > > I'm curious, why? Its 32bytes that is allocated a maximum of twice. I believe that I'm a stingy minimalist :) > (my questions on what needs to go in there were because it looked like a third > user was missing...) > > > > How about this? > > > > | struct kimage_arch { > > | phys_addr_t dtb_mem; > > | #ifdef CONFIG_KEXEC_FILE > > #ifdef in structs just breeds more #ifdefs, as the code that accesses those > members has to be behind the same set of conditions. > > Given this, I prefer the #ifdefs around cpu_soft_restart() as it doesn't force > us to add more #ifdefs later. OK > For either option without purgatory_info: > Reviewed-by: James Morse <james.morse@arm.com> Thanks, -Takahiro AKASHI > > Thanks, > > James
diff --git a/arch/arm64/kernel/cpu-reset.S b/arch/arm64/kernel/cpu-reset.S index 8021b46c9743..391df91328ac 100644 --- a/arch/arm64/kernel/cpu-reset.S +++ b/arch/arm64/kernel/cpu-reset.S @@ -24,9 +24,9 @@ * * @el2_switch: Flag to indicate a swich to EL2 is needed. * @entry: Location to jump to for soft reset. - * arg0: First argument passed to @entry. - * arg1: Second argument passed to @entry. - * arg2: Third argument passed to @entry. + * arg0: First argument passed to @entry. (relocation list) + * arg1: Second argument passed to @entry.(physcal kernel entry) + * arg2: Third argument passed to @entry. (physical dtb address) * * Put the CPU into the same state as it would be if it had been reset, and * branch to what would be the reset vector. It must be executed with the diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c index f76ea92dff91..f7dbba00be10 100644 --- a/arch/arm64/kernel/machine_kexec.c +++ b/arch/arm64/kernel/machine_kexec.c @@ -205,10 +205,17 @@ void machine_kexec(struct kimage *kimage) * uses physical addressing to relocate the new image to its final * position and transfers control to the image entry point when the * relocation is complete. + * In case of kexec_file_load syscall, we directly start the kernel, + * skipping purgatory. */ - cpu_soft_restart(kimage != kexec_crash_image, - reboot_code_buffer_phys, kimage->head, kimage->start, 0); + reboot_code_buffer_phys, kimage->head, kimage->start, +#ifdef CONFIG_KEXEC_FILE + kimage->purgatory_info.purgatory_buf ? + 0 : kimage->arch.dtb_mem); +#else + 0); +#endif BUG(); /* Should never get here. */ } diff --git a/arch/arm64/kernel/relocate_kernel.S b/arch/arm64/kernel/relocate_kernel.S index f407e422a720..95fd94209aae 100644 --- a/arch/arm64/kernel/relocate_kernel.S +++ b/arch/arm64/kernel/relocate_kernel.S @@ -32,6 +32,7 @@ ENTRY(arm64_relocate_new_kernel) /* Setup the list loop variables. */ + mov x18, x2 /* x18 = dtb address */ mov x17, x1 /* x17 = kimage_start */ mov x16, x0 /* x16 = kimage_head */ raw_dcache_line_size x15, x0 /* x15 = dcache line size */ @@ -107,7 +108,7 @@ ENTRY(arm64_relocate_new_kernel) isb /* Start new image. */ - mov x0, xzr + mov x0, x18 mov x1, xzr mov x2, xzr mov x3, xzr
On arm64, purugatory would do almosty nothing. So just invoke secondary kernel directy by jumping into its entry code. While, in this case, cpu_soft_restart() must be called with dtb address in the fifth argument, the behavior still stays compatible with kexec_load case as long as the argument is null. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will.deacon@arm.com> --- arch/arm64/kernel/cpu-reset.S | 6 +++--- arch/arm64/kernel/machine_kexec.c | 11 +++++++++-- arch/arm64/kernel/relocate_kernel.S | 3 ++- 3 files changed, 14 insertions(+), 6 deletions(-)