[RFC PATCH] ARM: kexec: Assemble relocate code in ARM mode

[RFC PATCH] ARM: kexec: Assemble relocate code in ARM mode

[Taras Kondratiuk]

In Thumb2 kernel (CONFIG_THUMB2_KERNEL) kexec's relocate code is assembled
in Thumb2 mode, but cpu_v7_reset() jumps to this code in ARM state,
because its address is page aligned and has 0 in LSB.

Assemble this code in ARM mode to fix the issue.

Signed-off-by: Taras Kondratiuk <taras.kondratiuk@linaro.org>
---
Based on v3.12-rc4

Cc: Dave Martin <dave.martin@linaro.org>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: linaro-kernel at lists.linaro.org
Cc: linux-arm-kernel at lists.infradead.org
---
 arch/arm/kernel/relocate_kernel.S |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/kernel/relocate_kernel.S b/arch/arm/kernel/relocate_kernel.S
index d0cdedf..a3af323 100644
--- a/arch/arm/kernel/relocate_kernel.S
+++ b/arch/arm/kernel/relocate_kernel.S
@@ -5,6 +5,7 @@
 #include <asm/kexec.h>
 
 	.globl relocate_new_kernel
+	.arm
 relocate_new_kernel:
 
 	ldr	r0,kexec_indirection_page
-- 
1.7.9.5

[RFC PATCH] ARM: kexec: Assemble relocate code in ARM mode

[Will Deacon]

On Wed, Oct 09, 2013 at 09:57:03PM +0100, Taras Kondratiuk wrote:
> In Thumb2 kernel (CONFIG_THUMB2_KERNEL) kexec's relocate code is assembled
> in Thumb2 mode, but cpu_v7_reset() jumps to this code in ARM state,
> because its address is page aligned and has 0 in LSB.

This used to work, but Dave broken it when he fixed the reset code in
153cd8e839b5 ("ARM: 7553/1: proc-v7: Ensure correct instruction set after
cpu_reset").

> Assemble this code in ARM mode to fix the issue.

In the interest of keeping kexec a possibility for v7m, it might be better
to do something similar to head.S (i.e. switch back to thumb if we're a
thumb-2 kernel).

Cheers,

Will

> Signed-off-by: Taras Kondratiuk <taras.kondratiuk@linaro.org>
> ---
> Based on v3.12-rc4
> 
> Cc: Dave Martin <dave.martin@linaro.org>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: linaro-kernel at lists.linaro.org
> Cc: linux-arm-kernel at lists.infradead.org
> ---
>  arch/arm/kernel/relocate_kernel.S |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/kernel/relocate_kernel.S b/arch/arm/kernel/relocate_kernel.S
> index d0cdedf..a3af323 100644
> --- a/arch/arm/kernel/relocate_kernel.S
> +++ b/arch/arm/kernel/relocate_kernel.S
> @@ -5,6 +5,7 @@
>  #include <asm/kexec.h>
>  
>  	.globl relocate_new_kernel
> +	.arm
>  relocate_new_kernel:
>  
>  	ldr	r0,kexec_indirection_page
> -- 
> 1.7.9.5
> 
> 

[RFC PATCH] ARM: kexec: Assemble relocate code in ARM mode

[Taras Kondratiuk]

On 10 October 2013 12:02, Will Deacon <will.deacon@arm.com> wrote:
> On Wed, Oct 09, 2013 at 09:57:03PM +0100, Taras Kondratiuk wrote:
>> In Thumb2 kernel (CONFIG_THUMB2_KERNEL) kexec's relocate code is assembled
>> in Thumb2 mode, but cpu_v7_reset() jumps to this code in ARM state,
>> because its address is page aligned and has 0 in LSB.
>
> This used to work, but Dave broken it when he fixed the reset code in
> 153cd8e839b5 ("ARM: 7553/1: proc-v7: Ensure correct instruction set after
> cpu_reset").
>
>> Assemble this code in ARM mode to fix the issue.
>
> In the interest of keeping kexec a possibility for v7m, it might be better
> to do something similar to head.S (i.e. switch back to thumb if we're a
> thumb-2 kernel).
>

Actually only v7 jumps here in ARM, so maybe it worth to wrap the patch with
CONFIG_CPU_V7? In this case v7m won't be touched.

I'll update patch with a stub that switches back to Thumb, but honestly
I don't see a bit sense of it here. Anyway at the end of this function
instruction set is switched again.

-- 
Regards,
Taras Kondratiuk

[RFC PATCH] ARM: kexec: Assemble relocate code in ARM mode

[Dave Martin]

On Wed, Oct 09, 2013 at 11:57:03PM +0300, Taras Kondratiuk wrote:
> In Thumb2 kernel (CONFIG_THUMB2_KERNEL) kexec's relocate code is assembled
> in Thumb2 mode, but cpu_v7_reset() jumps to this code in ARM state,
> because its address is page aligned and has 0 in LSB.
> 
> Assemble this code in ARM mode to fix the issue.

I think the actual issue here is that relocate_new_kernel is not properly
annotated as a function symbol.

Can you remove the explicit label declaration and try the following:

	#include <linux/linkage.h>

	ENTRY(relocate_new_kernel)

	/* body of relocate_new_kernel */

	ENDPROC(relocate_new_kernel)


Without this, the linker will treat it as a random pointer to data and
never set the Thumb bit.

This fails in precisely the same was as an ordinary function call
would fail if the destination function doesn't have the needed
annotation.


There should be no need to switch to ARM if the kernel is just jumping
to itself...

Cheers
---Dave

> 
> Signed-off-by: Taras Kondratiuk <taras.kondratiuk@linaro.org>
> ---
> Based on v3.12-rc4
> 
> Cc: Dave Martin <dave.martin@linaro.org>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: linaro-kernel at lists.linaro.org
> Cc: linux-arm-kernel at lists.infradead.org
> ---
>  arch/arm/kernel/relocate_kernel.S |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/kernel/relocate_kernel.S b/arch/arm/kernel/relocate_kernel.S
> index d0cdedf..a3af323 100644
> --- a/arch/arm/kernel/relocate_kernel.S
> +++ b/arch/arm/kernel/relocate_kernel.S
> @@ -5,6 +5,7 @@
>  #include <asm/kexec.h>
>  
>  	.globl relocate_new_kernel
> +	.arm
>  relocate_new_kernel:
>  
>  	ldr	r0,kexec_indirection_page
> -- 
> 1.7.9.5
> 

[RFC PATCH] ARM: kexec: Assemble relocate code in ARM mode

[Taras Kondratiuk]

On 10 October 2013 17:12, Dave Martin <Dave.Martin@arm.com> wrote:
> On Wed, Oct 09, 2013 at 11:57:03PM +0300, Taras Kondratiuk wrote:
>> In Thumb2 kernel (CONFIG_THUMB2_KERNEL) kexec's relocate code is assembled
>> in Thumb2 mode, but cpu_v7_reset() jumps to this code in ARM state,
>> because its address is page aligned and has 0 in LSB.
>>
>> Assemble this code in ARM mode to fix the issue.
>
> I think the actual issue here is that relocate_new_kernel is not properly
> annotated as a function symbol.
>
> Can you remove the explicit label declaration and try the following:
>
>         #include <linux/linkage.h>
>
>         ENTRY(relocate_new_kernel)
>
>         /* body of relocate_new_kernel */
>
>         ENDPROC(relocate_new_kernel)
>
>
> Without this, the linker will treat it as a random pointer to data and
> never set the Thumb bit.
>
> This fails in precisely the same was as an ordinary function call
> would fail if the destination function doesn't have the needed
> annotation.
>
>
> There should be no need to switch to ARM if the kernel is just jumping
> to itself...

I think it won't help, because here is no direct jump to this label.
This code gets copied to a new page and jump is done to the beginning
of that page.

[RFC PATCH] ARM: kexec: Assemble relocate code in ARM mode

[Taras Kondratiuk]

On 10/15/2013 06:24 PM, Dave Martin wrote:
> On Thu, Oct 10, 2013 at 11:36:08PM +0300, Taras Kondratiuk wrote:
>> I think it won't help, because here is no direct jump to this label.
>> This code gets copied to a new page and jump is done to the beginning
>> of that page.
> 
> Ah, right.
> 
> I think that the fncpy() function should work.  This is for the precise
> purpsoe of copying a function body from one place to another, while
> reatining the Thumb bit.
> 
> I have to disappear early to today, but I'll try and follow up.
> 
> Currently, I have the following, but I've not been able to test it yet.
> If you'd like to give it a try, that would be much appreciated.

I've tried the patch on ARM ISA and looks like the code does what
it should, but the reloaded kernel crashes somewhere at early stages.
Without this patch it boots fine on ARM. Next week I will try to look
what is going on.

> 
> From 94925667e650202e1baf74e1ff223671d316401d Mon Sep 17 00:00:00 2001
> From: Dave Martin <Dave.Martin@arm.com>
> Date: Tue, 15 Oct 2013 11:48:46 +0100
> Subject: [PATCH] ARM: kexec: Use the right ISA for relocate_new_kernel
> 
> Copying a function with memcpy() and then trying to execute the
> result isn't portable to Thumb.
> 
> This patch modifies the kexec soft restart code to copy its
> assembler trampoline relocate_new_kernel() using fncpy() instead,
> so that relocate_new_kernel can be in the same ISA as the rest of
> the kernel without problems.
> 
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> ---
>  arch/arm/kernel/machine_kexec.c   |   20 +++++++++-----------
>  arch/arm/kernel/relocate_kernel.S |    7 ++++---
>  2 files changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c
> index 57221e3..ce135b3 100644
> --- a/arch/arm/kernel/machine_kexec.c
> +++ b/arch/arm/kernel/machine_kexec.c
> @@ -11,6 +11,7 @@
>  #include <linux/memblock.h>
>  #include <asm/pgtable.h>
>  #include <linux/of_fdt.h>
> +#include <asm/fncpy.h>
>  #include <asm/pgalloc.h>
>  #include <asm/mmu_context.h>
>  #include <asm/cacheflush.h>
> @@ -18,7 +19,7 @@
>  #include <asm/smp_plat.h>
>  #include <asm/system_misc.h>
>  
> -extern const unsigned char relocate_new_kernel[];
> +extern char relocate_new_kernel;

Shouldn't it be a function pointer?
extern void relocate_new_kernel(void);

-- 
Taras Kondratiuk

[RFC PATCH] ARM: kexec: Assemble relocate code in ARM mode

[Dave Martin]

On Fri, Oct 18, 2013 at 10:29:41PM +0300, Taras Kondratiuk wrote:
> On 10/15/2013 06:24 PM, Dave Martin wrote:
> > On Thu, Oct 10, 2013 at 11:36:08PM +0300, Taras Kondratiuk wrote:
> >> I think it won't help, because here is no direct jump to this label.
> >> This code gets copied to a new page and jump is done to the beginning
> >> of that page.
> > 
> > Ah, right.
> > 
> > I think that the fncpy() function should work.  This is for the precise
> > purpsoe of copying a function body from one place to another, while
> > reatining the Thumb bit.
> > 
> > I have to disappear early to today, but I'll try and follow up.
> > 
> > Currently, I have the following, but I've not been able to test it yet.
> > If you'd like to give it a try, that would be much appreciated.
> 
> I've tried the patch on ARM ISA and looks like the code does what

Thanks for giving it a try.

> it should, but the reloaded kernel crashes somewhere at early stages.
> Without this patch it boots fine on ARM. Next week I will try to look
> what is going on.

Hmmm, that's a bit strange.  You're saying that the new kernel gets
loaded and started OK, but crashes during boot?

If relocate_new_kernel() was failing, I would not expect the new
kernel to boot at all, unless part of the kernel doesn't get relocated,
or something like that.

> > 
> > From 94925667e650202e1baf74e1ff223671d316401d Mon Sep 17 00:00:00 2001
> > From: Dave Martin <Dave.Martin@arm.com>
> > Date: Tue, 15 Oct 2013 11:48:46 +0100
> > Subject: [PATCH] ARM: kexec: Use the right ISA for relocate_new_kernel
> > 
> > Copying a function with memcpy() and then trying to execute the
> > result isn't portable to Thumb.
> > 
> > This patch modifies the kexec soft restart code to copy its
> > assembler trampoline relocate_new_kernel() using fncpy() instead,
> > so that relocate_new_kernel can be in the same ISA as the rest of
> > the kernel without problems.
> > 
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > ---
> >  arch/arm/kernel/machine_kexec.c   |   20 +++++++++-----------
> >  arch/arm/kernel/relocate_kernel.S |    7 ++++---
> >  2 files changed, 13 insertions(+), 14 deletions(-)
> > 
> > diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c
> > index 57221e3..ce135b3 100644
> > --- a/arch/arm/kernel/machine_kexec.c
> > +++ b/arch/arm/kernel/machine_kexec.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/memblock.h>
> >  #include <asm/pgtable.h>
> >  #include <linux/of_fdt.h>
> > +#include <asm/fncpy.h>
> >  #include <asm/pgalloc.h>
> >  #include <asm/mmu_context.h>
> >  #include <asm/cacheflush.h>
> > @@ -18,7 +19,7 @@
> >  #include <asm/smp_plat.h>
> >  #include <asm/system_misc.h>
> >  
> > -extern const unsigned char relocate_new_kernel[];
> > +extern char relocate_new_kernel;
> 
> Shouldn't it be a function pointer?
> extern void relocate_new_kernel(void);

Actually, it could be.

I was forgetting that fncpy was implemented as a macro, allowing
it to be more forgiving about argument types.

Casting a function pointer type to a non-function pointer type is
illegal in C, but fncpy dodges that problem by hiding the cast
inside an asm so that the compiler can't try to do clever optimisations
that might break things.

Cheers
---Dave