Patchwork [lucid] UBUNTU: SAUCE: dove: avoid page table overwrite when resuming from hibernation

login
register
mail settings
Submitter Eric Miao
Date April 28, 2010, 3 a.m.
Message ID <v2wf17812d71004272000v8a8633b8r995e9a50cef1e483@mail.gmail.com>
Download mbox | patch
Permalink /patch/51144/
State Superseded
Delegated to: Stefan Bader
Headers show

Comments

Eric Miao - April 28, 2010, 3 a.m.
> Hm, wouldn't it then be possible to make the patch actually use the macro name
> instead of the hard coded values?
>

How about the patch below, making thing a little bit clearer?

commit ee5c472440e089eba863689c446adb5f1c0ae05e
Author: Eric Miao <eric.miao@canonical.com>
Date:   Fri Apr 23 14:16:17 2010 +0800

    UBUNTU: SAUCE: dove: avoid page table overwrite when resuming from
hibernation

    BugLink: http://bugs.launchpad.net/bugs/509006

    Resuming from hibernation is OK if 'resume=/dev/sdaX' is explicitly
    specified on the kernel command line, but it fails if scripts in
    initramfs are used to trigger the resume. It turned out to be page
    table being overwritten when restoring the memory content because
    it's using a normal user process's page table in the latter case,
    which is not safe and could be overwritten. Fix this by using the
    safe swapper_pg_dir during restoring.

    Signed-off-by: Eric Miao <eric.miao@canonical.com>
Bryan Wu - April 28, 2010, 5:56 a.m.
On 04/28/2010 11:00 AM, Eric Miao wrote:
>> Hm, wouldn't it then be possible to make the patch actually use the macro name
>> instead of the hard coded values?
>>
>
> How about the patch below, making thing a little bit clearer?
>
> commit ee5c472440e089eba863689c446adb5f1c0ae05e
> Author: Eric Miao<eric.miao@canonical.com>
> Date:   Fri Apr 23 14:16:17 2010 +0800
>
>      UBUNTU: SAUCE: dove: avoid page table overwrite when resuming from
> hibernation
>
>      BugLink: http://bugs.launchpad.net/bugs/509006
>
>      Resuming from hibernation is OK if 'resume=/dev/sdaX' is explicitly
>      specified on the kernel command line, but it fails if scripts in
>      initramfs are used to trigger the resume. It turned out to be page
>      table being overwritten when restoring the memory content because
>      it's using a normal user process's page table in the latter case,
>      which is not safe and could be overwritten. Fix this by using the
>      safe swapper_pg_dir during restoring.
>
>      Signed-off-by: Eric Miao<eric.miao@canonical.com>
>
> diff --git a/arch/arm/mach-dove/Makefile b/arch/arm/mach-dove/Makefile
> index 0be1e1c..c5c028f 100755
> --- a/arch/arm/mach-dove/Makefile
> +++ b/arch/arm/mach-dove/Makefile
> @@ -1,3 +1,5 @@
> +AFLAGS_swsusp.o			:= -DTEXT_OFFSET=$(TEXT_OFFSET)
> +
>   obj-y				+= clock.o common.o addr-map.o irq.o pcie.o mpp.o \
>   				sdhci_cam_mbus.o
>   obj-$(CONFIG_MACH_DOVE_RD_AVNG)	+= dove-rd-avng-setup.o
> diff --git a/arch/arm/mach-dove/swsusp.S b/arch/arm/mach-dove/swsusp.S
> index 4f4a884..8e308d8 100644
> --- a/arch/arm/mach-dove/swsusp.S
> +++ b/arch/arm/mach-dove/swsusp.S
> @@ -28,6 +28,7 @@
>    */
>
>   #include<linux/linkage.h>
> +#include<asm/memory.h>
>   #include<asm/segment.h>
>   #include<asm/page.h>
>   #include<asm/asm-offsets.h>
> @@ -209,8 +210,14 @@ FUNC(swsusp_arch_suspend)
>
>   FUNC_END(swsusp_arch_suspend)
>
> +#define KERNEL_RAM_PADDR	(PHYS_OFFSET + TEXT_OFFSET)
> +
>   FUNC(swsusp_arch_resume)
>   	/* set page table if needed */
> +	ldr	r0, =(KERNEL_RAM_PADDR - 0x4000)

How about using the pgtbl marco as you posted in previous email?

   +	pgtbl   r0			@ page table address

I think Stephan was asking for this. But if 'pgtbl' is not available in this ASM 
file, I am OK with this patch.

> +	mcr	p15, 0, r0, c2, c0, 0		@ load page table pointer
> +	mcr	p15, 0, r0, c8, c7, 0		@ invalidate I,D TLBs
> +	mcr	p15, 0, r0, c7, c5, 4		@ ISB
>
>   	/*
>   	 * retore "nr_copy_pages" pages which are saved and specified
>

Thanks,
Eric Miao - April 28, 2010, 6:12 a.m.
On Wed, Apr 28, 2010 at 1:56 PM, Bryan Wu <bryan.wu@canonical.com> wrote:
> On 04/28/2010 11:00 AM, Eric Miao wrote:
>>>
>>> Hm, wouldn't it then be possible to make the patch actually use the macro
>>> name
>>> instead of the hard coded values?
>>>
>>
>> How about the patch below, making thing a little bit clearer?
>>
>> commit ee5c472440e089eba863689c446adb5f1c0ae05e
>> Author: Eric Miao<eric.miao@canonical.com>
>> Date:   Fri Apr 23 14:16:17 2010 +0800
>>
>>     UBUNTU: SAUCE: dove: avoid page table overwrite when resuming from
>> hibernation
>>
>>     BugLink: http://bugs.launchpad.net/bugs/509006
>>
>>     Resuming from hibernation is OK if 'resume=/dev/sdaX' is explicitly
>>     specified on the kernel command line, but it fails if scripts in
>>     initramfs are used to trigger the resume. It turned out to be page
>>     table being overwritten when restoring the memory content because
>>     it's using a normal user process's page table in the latter case,
>>     which is not safe and could be overwritten. Fix this by using the
>>     safe swapper_pg_dir during restoring.
>>
>>     Signed-off-by: Eric Miao<eric.miao@canonical.com>
>>
>> diff --git a/arch/arm/mach-dove/Makefile b/arch/arm/mach-dove/Makefile
>> index 0be1e1c..c5c028f 100755
>> --- a/arch/arm/mach-dove/Makefile
>> +++ b/arch/arm/mach-dove/Makefile
>> @@ -1,3 +1,5 @@
>> +AFLAGS_swsusp.o                        := -DTEXT_OFFSET=$(TEXT_OFFSET)
>> +
>>  obj-y                         += clock.o common.o addr-map.o irq.o pcie.o
>> mpp.o \
>>                                sdhci_cam_mbus.o
>>  obj-$(CONFIG_MACH_DOVE_RD_AVNG)       += dove-rd-avng-setup.o
>> diff --git a/arch/arm/mach-dove/swsusp.S b/arch/arm/mach-dove/swsusp.S
>> index 4f4a884..8e308d8 100644
>> --- a/arch/arm/mach-dove/swsusp.S
>> +++ b/arch/arm/mach-dove/swsusp.S
>> @@ -28,6 +28,7 @@
>>   */
>>
>>  #include<linux/linkage.h>
>> +#include<asm/memory.h>
>>  #include<asm/segment.h>
>>  #include<asm/page.h>
>>  #include<asm/asm-offsets.h>
>> @@ -209,8 +210,14 @@ FUNC(swsusp_arch_suspend)
>>
>>  FUNC_END(swsusp_arch_suspend)
>>
>> +#define KERNEL_RAM_PADDR       (PHYS_OFFSET + TEXT_OFFSET)
>> +
>>  FUNC(swsusp_arch_resume)
>>        /* set page table if needed */
>> +       ldr     r0, =(KERNEL_RAM_PADDR - 0x4000)
>
> How about using the pgtbl marco as you posted in previous email?
>
>  +     pgtbl   r0                      @ page table address
>
> I think Stephan was asking for this. But if 'pgtbl' is not available in this
> ASM file, I am OK with this patch.
>

Yeah, that's a better approach and I was originally thinking of that, but it
needs this macro to be declared somewhere in a assembly header.

Patch

diff --git a/arch/arm/mach-dove/Makefile b/arch/arm/mach-dove/Makefile
index 0be1e1c..c5c028f 100755
--- a/arch/arm/mach-dove/Makefile
+++ b/arch/arm/mach-dove/Makefile
@@ -1,3 +1,5 @@ 
+AFLAGS_swsusp.o			:= -DTEXT_OFFSET=$(TEXT_OFFSET)
+
 obj-y				+= clock.o common.o addr-map.o irq.o pcie.o mpp.o \
 				sdhci_cam_mbus.o
 obj-$(CONFIG_MACH_DOVE_RD_AVNG)	+= dove-rd-avng-setup.o
diff --git a/arch/arm/mach-dove/swsusp.S b/arch/arm/mach-dove/swsusp.S
index 4f4a884..8e308d8 100644
--- a/arch/arm/mach-dove/swsusp.S
+++ b/arch/arm/mach-dove/swsusp.S
@@ -28,6 +28,7 @@ 
  */

 #include <linux/linkage.h>
+#include <asm/memory.h>
 #include <asm/segment.h>
 #include <asm/page.h>
 #include <asm/asm-offsets.h>
@@ -209,8 +210,14 @@  FUNC(swsusp_arch_suspend)

 FUNC_END(swsusp_arch_suspend)

+#define KERNEL_RAM_PADDR	(PHYS_OFFSET + TEXT_OFFSET)
+
 FUNC(swsusp_arch_resume)
 	/* set page table if needed */
+	ldr	r0, =(KERNEL_RAM_PADDR - 0x4000)
+	mcr	p15, 0, r0, c2, c0, 0		@ load page table pointer
+	mcr	p15, 0, r0, c8, c7, 0		@ invalidate I,D TLBs
+	mcr	p15, 0, r0, c7, c5, 4		@ ISB

 	/*
 	 * retore "nr_copy_pages" pages which are saved and specified