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

Message ID 20131015152422.GC2312@localhost.localdomain
State New
Headers show

Commit Message

Dave Martin Oct. 15, 2013, 3:24 p.m.
On Thu, Oct 10, 2013 at 11:36:08PM +0300, Taras Kondratiuk wrote:
> 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.

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.

Cheers
---Dave

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(-)

Comments

Taras Kondratiuk Oct. 18, 2013, 7:29 p.m. | #1
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);
Dave Martin Oct. 24, 2013, 2:34 p.m. | #2
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

Patch hide | download patch | download mbox

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;
 extern const unsigned int relocate_new_kernel_size;
 
 extern unsigned long kexec_start_address;
@@ -141,8 +142,9 @@  void (*kexec_reinit)(void);
 void machine_kexec(struct kimage *image)
 {
 	unsigned long page_list;
-	unsigned long reboot_code_buffer_phys;
 	void *reboot_code_buffer;
+	unsigned long v2p_offset;
+	void *entry_point;
 
 	/*
 	 * This can only happen if machine_shutdown() failed to disable some
@@ -154,10 +156,9 @@  void machine_kexec(struct kimage *image)
 
 	page_list = image->head & PAGE_MASK;
 
-	/* we need both effective and real address here */
-	reboot_code_buffer_phys =
-	    page_to_pfn(image->control_code_page) << PAGE_SHIFT;
 	reboot_code_buffer = page_address(image->control_code_page);
+	v2p_offset = (page_to_pfn(image->control_code_page) << PAGE_SHIFT)
+		- (unsigned long)reboot_code_buffer;
 
 	/* Prepare parameters for reboot_code_buffer*/
 	kexec_start_address = image->start;
@@ -168,16 +169,13 @@  void machine_kexec(struct kimage *image)
 
 
 	/* copy our kernel relocation code to the control code page */
-	memcpy(reboot_code_buffer,
-	       relocate_new_kernel, relocate_new_kernel_size);
+	entry_point = fncpy(reboot_code_buffer,
+			    &relocate_new_kernel, relocate_new_kernel_size);
 
-
-	flush_icache_range((unsigned long) reboot_code_buffer,
-			   (unsigned long) reboot_code_buffer + KEXEC_CONTROL_PAGE_SIZE);
 	printk(KERN_INFO "Bye!\n");
 
 	if (kexec_reinit)
 		kexec_reinit();
 
-	soft_restart(reboot_code_buffer_phys);
+	soft_restart((unsigned long)entry_point + v2p_offset);
 }
diff --git a/arch/arm/kernel/relocate_kernel.S b/arch/arm/kernel/relocate_kernel.S
index d0cdedf..1492930 100644
--- a/arch/arm/kernel/relocate_kernel.S
+++ b/arch/arm/kernel/relocate_kernel.S
@@ -2,11 +2,11 @@ 
  * relocate_kernel.S - put the kernel image in place to boot
  */
 
+#include <linux/linkage.h>
 #include <asm/kexec.h>
 
-	.globl relocate_new_kernel
-relocate_new_kernel:
-
+	.align	3
+ENTRY(relocate_new_kernel)
 	ldr	r0,kexec_indirection_page
 	ldr	r1,kexec_start_address
 
@@ -59,6 +59,7 @@  relocate_new_kernel:
 	ldr r2,kexec_boot_atags
  ARM(	mov pc, lr	)
  THUMB(	bx lr		)
+ENDPROC(relocate_new_kernel)
 
 	.align