[PATCH] ARM: kdump: copy kernel relocation code at the kexec prepare stage

[PATCH] ARM: kdump: copy kernel relocation code at the kexec prepare stage

[Lei Wen]

This copy really don't need to do at the very second before the kernel
would crash.

Signed-off-by: Lei Wen <leiwen@marvell.com>
---
 arch/arm/kernel/machine_kexec.c |   30 +++++++++++++++---------------
 1 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c
index e59bbd4..f60fc90 100644
--- a/arch/arm/kernel/machine_kexec.c
+++ b/arch/arm/kernel/machine_kexec.c
@@ -32,6 +32,21 @@ static atomic_t waiting_for_crash_ipi;
 
 int machine_kexec_prepare(struct kimage *image)
 {
+	unsigned long page_list;
+	void *reboot_code_buffer;
+	page_list = image->head & PAGE_MASK;
+
+	reboot_code_buffer = page_address(image->control_code_page);
+
+	/* Prepare parameters for reboot_code_buffer*/
+	kexec_start_address = image->start;
+	kexec_indirection_page = page_list;
+	kexec_mach_type = machine_arch_type;
+	kexec_boot_atags = image->start - KEXEC_ARM_ZIMAGE_OFFSET + KEXEC_ARM_ATAGS_OFFSET;
+
+	/* copy our kernel relocation code to the control code page */
+	memcpy(reboot_code_buffer,
+	       relocate_new_kernel, relocate_new_kernel_size);
 	return 0;
 }
 
@@ -82,29 +97,14 @@ void (*kexec_reinit)(void);
 
 void machine_kexec(struct kimage *image)
 {
-	unsigned long page_list;
 	unsigned long reboot_code_buffer_phys;
 	void *reboot_code_buffer;
 
-
-	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);
 
-	/* Prepare parameters for reboot_code_buffer*/
-	kexec_start_address = image->start;
-	kexec_indirection_page = page_list;
-	kexec_mach_type = machine_arch_type;
-	kexec_boot_atags = image->start - KEXEC_ARM_ZIMAGE_OFFSET + KEXEC_ARM_ATAGS_OFFSET;
-
-	/* copy our kernel relocation code to the control code page */
-	memcpy(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");
-- 
1.7.0.4

[PATCH] ARM: kdump: copy kernel relocation code at the kexec prepare stage

[Simon Horman]

On Thu, Sep 15, 2011 at 10:32:09PM -0700, Lei Wen wrote:
> This copy really don't need to do at the very second before the kernel
> would crash.
> 
> Signed-off-by: Lei Wen <leiwen@marvell.com>

This seems reasonable to me.

Acked-by: Simon Horman <horms@verge.net.au>

[PATCH] ARM: kdump: copy kernel relocation code at the kexec prepare stage

[Lei Wen]

Hi Simon,

On Fri, Sep 16, 2011 at 1:45 PM, Simon Horman <horms@verge.net.au> wrote:
> On Thu, Sep 15, 2011 at 10:32:09PM -0700, Lei Wen wrote:
>> This copy really don't need to do at the very second before the kernel
>> would crash.
>>
>> Signed-off-by: Lei Wen <leiwen@marvell.com>
>
> This seems reasonable to me.
>
> Acked-by: Simon Horman <horms@verge.net.au>

Thanks for acked. :)

BTW, do you know which git tree should this patch be merged?

Best regards,
Lei

[PATCH] ARM: kdump: copy kernel relocation code at the kexec prepare stage

[Lei Wen]

On Sat, Sep 17, 2011 at 9:58 PM, Lei Wen <adrian.wenl@gmail.com> wrote:
> Hi Simon,
>
> On Fri, Sep 16, 2011 at 1:45 PM, Simon Horman <horms@verge.net.au> wrote:
>> On Thu, Sep 15, 2011 at 10:32:09PM -0700, Lei Wen wrote:
>>> This copy really don't need to do at the very second before the kernel
>>> would crash.
>>>
>>> Signed-off-by: Lei Wen <leiwen@marvell.com>
>>
>> This seems reasonable to me.
>>
>> Acked-by: Simon Horman <horms@verge.net.au>
>
> Thanks for acked. :)
>
> BTW, do you know which git tree should this patch be merged?
>

Push it to Russell's patch tracking system. :)

Best regards,
Lei

[PATCH] ARM: kdump: copy kernel relocation code at the kexec prepare stage

[Russell King - ARM Linux]

On Thu, Sep 15, 2011 at 10:32:09PM -0700, Lei Wen wrote:
> diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c
> index e59bbd4..f60fc90 100644
> --- a/arch/arm/kernel/machine_kexec.c
> +++ b/arch/arm/kernel/machine_kexec.c
> @@ -32,6 +32,21 @@ static atomic_t waiting_for_crash_ipi;
>  
>  int machine_kexec_prepare(struct kimage *image)
>  {
> +	unsigned long page_list;
> +	void *reboot_code_buffer;
> +	page_list = image->head & PAGE_MASK;
> +
> +	reboot_code_buffer = page_address(image->control_code_page);
> +
> +	/* Prepare parameters for reboot_code_buffer*/
> +	kexec_start_address = image->start;
> +	kexec_indirection_page = page_list;
> +	kexec_mach_type = machine_arch_type;
> +	kexec_boot_atags = image->start - KEXEC_ARM_ZIMAGE_OFFSET + KEXEC_ARM_ATAGS_OFFSET;
> +
> +	/* copy our kernel relocation code to the control code page */
> +	memcpy(reboot_code_buffer,
> +	       relocate_new_kernel, relocate_new_kernel_size);
>  	return 0;
>  }
>  
> @@ -82,29 +97,14 @@ void (*kexec_reinit)(void);
>  
>  void machine_kexec(struct kimage *image)
>  {
> -	unsigned long page_list;
>  	unsigned long reboot_code_buffer_phys;
>  	void *reboot_code_buffer;
>  
> -
> -	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);
>  
> -	/* Prepare parameters for reboot_code_buffer*/
> -	kexec_start_address = image->start;
> -	kexec_indirection_page = page_list;
> -	kexec_mach_type = machine_arch_type;
> -	kexec_boot_atags = image->start - KEXEC_ARM_ZIMAGE_OFFSET + KEXEC_ARM_ATAGS_OFFSET;
> -
> -	/* copy our kernel relocation code to the control code page */
> -	memcpy(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);

You should keep this flush with the memcpy - the two are intimately
related - the flush is to ensure I/D cache coherency for the code
which was copied into the page.  Splitting them into two different
functions is asking for future bugs.

[PATCH] ARM: kdump: copy kernel relocation code at the kexec prepare stage

[Lei Wen]

Hi Russell,

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Monday, September 19, 2011 8:21 PM
> To: Lei Wen
> Cc: Chao Xie; Yu Tang; linux-arm-kernel at lists.infradead.org;
> kexec at lists.infradead.org
> Subject: Re: [PATCH] ARM: kdump: copy kernel relocation code at the kexec prepare
> stage
> 
> On Thu, Sep 15, 2011 at 10:32:09PM -0700, Lei Wen wrote:
> > diff --git a/arch/arm/kernel/machine_kexec.c
> b/arch/arm/kernel/machine_kexec.c
> > index e59bbd4..f60fc90 100644
> > --- a/arch/arm/kernel/machine_kexec.c
> > +++ b/arch/arm/kernel/machine_kexec.c
> > @@ -32,6 +32,21 @@ static atomic_t waiting_for_crash_ipi;
> >
> >  int machine_kexec_prepare(struct kimage *image)
> >  {
> > +	unsigned long page_list;
> > +	void *reboot_code_buffer;
> > +	page_list = image->head & PAGE_MASK;
> > +
> > +	reboot_code_buffer = page_address(image->control_code_page);
> > +
> > +	/* Prepare parameters for reboot_code_buffer*/
> > +	kexec_start_address = image->start;
> > +	kexec_indirection_page = page_list;
> > +	kexec_mach_type = machine_arch_type;
> > +	kexec_boot_atags = image->start - KEXEC_ARM_ZIMAGE_OFFSET +
> KEXEC_ARM_ATAGS_OFFSET;
> > +
> > +	/* copy our kernel relocation code to the control code page */
> > +	memcpy(reboot_code_buffer,
> > +	       relocate_new_kernel, relocate_new_kernel_size);
> >  	return 0;
> >  }
> >
> > @@ -82,29 +97,14 @@ void (*kexec_reinit)(void);
> >
> >  void machine_kexec(struct kimage *image)
> >  {
> > -	unsigned long page_list;
> >  	unsigned long reboot_code_buffer_phys;
> >  	void *reboot_code_buffer;
> >
> > -
> > -	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);
> >
> > -	/* Prepare parameters for reboot_code_buffer*/
> > -	kexec_start_address = image->start;
> > -	kexec_indirection_page = page_list;
> > -	kexec_mach_type = machine_arch_type;
> > -	kexec_boot_atags = image->start - KEXEC_ARM_ZIMAGE_OFFSET +
> KEXEC_ARM_ATAGS_OFFSET;
> > -
> > -	/* copy our kernel relocation code to the control code page */
> > -	memcpy(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);
> 
> You should keep this flush with the memcpy - the two are intimately
> related - the flush is to ensure I/D cache coherency for the code
> which was copied into the page.  Splitting them into two different
> functions is asking for future bugs.

Thanks for this reminding!
Then how about add flush_icache_range at the end of machine_kexec_prepare? Could it be acceptable?

Also with this change, do we still need flush icache at the end stage of machine_kexec?

Thanks,
Lei

[PATCH] ARM: kdump: copy kernel relocation code at the kexec prepare stage

[Russell King - ARM Linux]

On Mon, Sep 19, 2011 at 07:11:59AM -0700, Lei Wen wrote:
> Thanks for this reminding!
> Then how about add flush_icache_range at the end of machine_kexec_prepare?
> Could it be acceptable?

I think it should be done.

> Also with this change, do we still need flush icache at the end stage of
> machine_kexec?

You need to use flush_icache_range() whenever you write _instructions_
to an area of memory which you intend to execute.  If you're not writing
instructions, you shouldn't use flush_icache_range().

[PATCH V2] ARM: kdump: copy kernel relocation code at the kexec prepare stage

[Lei Wen]

This copy really don't need to do at the very second before the kernel
would crash.

Signed-off-by: Lei Wen <leiwen@marvell.com>
Acked-by: Simon Horman <horms@verge.net.au>
---
Changelog:
V2: move the flush_icache_range after the memcpy operation to ensure I/D cache
    coherency for the relocation code

 arch/arm/kernel/machine_kexec.c |   35 ++++++++++++++++++-----------------
 1 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c
index e59bbd4..c1b4463 100644
--- a/arch/arm/kernel/machine_kexec.c
+++ b/arch/arm/kernel/machine_kexec.c
@@ -32,6 +32,24 @@ static atomic_t waiting_for_crash_ipi;
 
 int machine_kexec_prepare(struct kimage *image)
 {
+	unsigned long page_list;
+	void *reboot_code_buffer;
+	page_list = image->head & PAGE_MASK;
+
+	reboot_code_buffer = page_address(image->control_code_page);
+
+	/* Prepare parameters for reboot_code_buffer*/
+	kexec_start_address = image->start;
+	kexec_indirection_page = page_list;
+	kexec_mach_type = machine_arch_type;
+	kexec_boot_atags = image->start - KEXEC_ARM_ZIMAGE_OFFSET + KEXEC_ARM_ATAGS_OFFSET;
+
+	/* copy our kernel relocation code to the control code page */
+	memcpy(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);
 	return 0;
 }
 
@@ -82,31 +100,14 @@ void (*kexec_reinit)(void);
 
 void machine_kexec(struct kimage *image)
 {
-	unsigned long page_list;
 	unsigned long reboot_code_buffer_phys;
 	void *reboot_code_buffer;
 
-
-	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);
 
-	/* Prepare parameters for reboot_code_buffer*/
-	kexec_start_address = image->start;
-	kexec_indirection_page = page_list;
-	kexec_mach_type = machine_arch_type;
-	kexec_boot_atags = image->start - KEXEC_ARM_ZIMAGE_OFFSET + KEXEC_ARM_ATAGS_OFFSET;
-
-	/* copy our kernel relocation code to the control code page */
-	memcpy(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)
-- 
1.7.0.4

[PATCH V2] ARM: kdump: copy kernel relocation code at the kexec prepare stage

[Russell King - ARM Linux]

On Tue, Sep 20, 2011 at 08:00:43PM +0800, Lei Wen wrote:
> This copy really don't need to do at the very second before the kernel
> would crash.
> 
> Signed-off-by: Lei Wen <leiwen@marvell.com>
> Acked-by: Simon Horman <horms@verge.net.au>
> ---
> Changelog:
> V2: move the flush_icache_range after the memcpy operation to ensure I/D cache
>     coherency for the relocation code

Much better, thanks.