diff mbox series

[v9,03/11] arm64: kexec_file: invoke the kernel without purgatory

Message ID 20180425062629.29404-4-takahiro.akashi@linaro.org
State New
Headers show
Series arm64: kexec: add kexec_file_load() support | expand

Commit Message

AKASHI Takahiro April 25, 2018, 6:26 a.m. UTC 
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(-)

-- 
2.17.0

Comments

James Morse May 1, 2018, 5:46 p.m. UTC | #1 
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
AKASHI Takahiro May 7, 2018, 5:22 a.m. UTC | #2 
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
James Morse May 11, 2018, 5:03 p.m. UTC | #3 
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
AKASHI Takahiro May 15, 2018, 4:45 a.m. UTC | #4 
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
James Morse May 15, 2018, 4:15 p.m. UTC | #5 
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
AKASHI Takahiro May 18, 2018, 6:22 a.m. UTC | #6 
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 mbox series

Patch

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