[U-Boot,v2,20/22] x86: acpi: Fix Windows S3 resume failure

Message ID 1492784689-15701-21-git-send-email-bmeng.cn@gmail.com
State Accepted
Commit 5ae5aa931042bb4163c6343a3cba65e95d90205a
Delegated to: Bin Meng
Headers show

Commit Message

Bin Meng April 21, 2017, 2:24 p.m.
U-Boot sets up the real mode interrupt handler stubs starting from
address 0x1000. In most cases, the first 640K (0x00000 - 0x9ffff)
system memory is reported as system RAM in E820 table to the OS.
(see install_e820_map() implementation for each platform). So OS
can use these memories whatever it wants.

If U-Boot is in an S3 resume path, care must be taken not to corrupt
these memorie otherwise OS data gets lost. Testing shows that, on
Microsoft Windows 10 on Intel Baytrail its wake up vector happens to
be installed at the same address 0x1000. While on Linux its wake up
vector does not overlap this memory range, but after resume kernel
checks low memory range per config option CONFIG_X86_RESERVE_LOW
which is 64K by default to see whether a memory corruption occurs
during the suspend/resume (it's harmless, but warnings are shown
in the kernel dmesg logs).

We cannot simply mark the these memory as reserved in E820 table
because such configuration makes GRUB complain: unable to allocate
real mode page. Hence we choose to back up these memories to the
place where we reserved on our stack for our S3 resume work.
Before jumping to OS wake up vector, we need restore the original
content there.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

---

Changes in v2:
- new patch "Fix Windows S3 resume failure"

 arch/x86/cpu/cpu.c                 |  8 +++++--
 arch/x86/include/asm/acpi_s3.h     | 16 +++++++++++++
 arch/x86/include/asm/global_data.h |  1 +
 arch/x86/lib/acpi_s3.c             | 48 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 71 insertions(+), 2 deletions(-)

Comments

Simon Glass April 24, 2017, 3:38 a.m. | #1
Hi Bin,

On 21 April 2017 at 08:24, Bin Meng <bmeng.cn@gmail.com> wrote:
> U-Boot sets up the real mode interrupt handler stubs starting from
> address 0x1000. In most cases, the first 640K (0x00000 - 0x9ffff)
> system memory is reported as system RAM in E820 table to the OS.
> (see install_e820_map() implementation for each platform). So OS
> can use these memories whatever it wants.
>
> If U-Boot is in an S3 resume path, care must be taken not to corrupt
> these memorie otherwise OS data gets lost. Testing shows that, on
> Microsoft Windows 10 on Intel Baytrail its wake up vector happens to
> be installed at the same address 0x1000. While on Linux its wake up
> vector does not overlap this memory range, but after resume kernel
> checks low memory range per config option CONFIG_X86_RESERVE_LOW
> which is 64K by default to see whether a memory corruption occurs
> during the suspend/resume (it's harmless, but warnings are shown
> in the kernel dmesg logs).
>
> We cannot simply mark the these memory as reserved in E820 table
> because such configuration makes GRUB complain: unable to allocate
> real mode page. Hence we choose to back up these memories to the
> place where we reserved on our stack for our S3 resume work.
> Before jumping to OS wake up vector, we need restore the original
> content there.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>
> ---
>
> Changes in v2:
> - new patch "Fix Windows S3 resume failure"
>
>  arch/x86/cpu/cpu.c                 |  8 +++++--
>  arch/x86/include/asm/acpi_s3.h     | 16 +++++++++++++
>  arch/x86/include/asm/global_data.h |  1 +
>  arch/x86/lib/acpi_s3.c             | 48 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 71 insertions(+), 2 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

But can you use a #define for the 0x1000 address?

Regards,
Simon
Bin Meng April 25, 2017, 1:51 a.m. | #2
Hi Simon,

On Mon, Apr 24, 2017 at 11:38 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Bin,
>
> On 21 April 2017 at 08:24, Bin Meng <bmeng.cn@gmail.com> wrote:
>> U-Boot sets up the real mode interrupt handler stubs starting from
>> address 0x1000. In most cases, the first 640K (0x00000 - 0x9ffff)
>> system memory is reported as system RAM in E820 table to the OS.
>> (see install_e820_map() implementation for each platform). So OS
>> can use these memories whatever it wants.
>>
>> If U-Boot is in an S3 resume path, care must be taken not to corrupt
>> these memorie otherwise OS data gets lost. Testing shows that, on
>> Microsoft Windows 10 on Intel Baytrail its wake up vector happens to
>> be installed at the same address 0x1000. While on Linux its wake up
>> vector does not overlap this memory range, but after resume kernel
>> checks low memory range per config option CONFIG_X86_RESERVE_LOW
>> which is 64K by default to see whether a memory corruption occurs
>> during the suspend/resume (it's harmless, but warnings are shown
>> in the kernel dmesg logs).
>>
>> We cannot simply mark the these memory as reserved in E820 table
>> because such configuration makes GRUB complain: unable to allocate
>> real mode page. Hence we choose to back up these memories to the
>> place where we reserved on our stack for our S3 resume work.
>> Before jumping to OS wake up vector, we need restore the original
>> content there.
>>
>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>>
>> ---
>>
>> Changes in v2:
>> - new patch "Fix Windows S3 resume failure"
>>
>>  arch/x86/cpu/cpu.c                 |  8 +++++--
>>  arch/x86/include/asm/acpi_s3.h     | 16 +++++++++++++
>>  arch/x86/include/asm/global_data.h |  1 +
>>  arch/x86/lib/acpi_s3.c             | 48 ++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 71 insertions(+), 2 deletions(-)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> But can you use a #define for the 0x1000 address?
>

Will prepare another patch to change this globally.

Regards,
Bin
Bin Meng April 26, 2017, 7:35 a.m. | #3
On Mon, Apr 24, 2017 at 11:38 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Bin,
>
> On 21 April 2017 at 08:24, Bin Meng <bmeng.cn@gmail.com> wrote:
>> U-Boot sets up the real mode interrupt handler stubs starting from
>> address 0x1000. In most cases, the first 640K (0x00000 - 0x9ffff)
>> system memory is reported as system RAM in E820 table to the OS.
>> (see install_e820_map() implementation for each platform). So OS
>> can use these memories whatever it wants.
>>
>> If U-Boot is in an S3 resume path, care must be taken not to corrupt
>> these memorie otherwise OS data gets lost. Testing shows that, on
>> Microsoft Windows 10 on Intel Baytrail its wake up vector happens to
>> be installed at the same address 0x1000. While on Linux its wake up
>> vector does not overlap this memory range, but after resume kernel
>> checks low memory range per config option CONFIG_X86_RESERVE_LOW
>> which is 64K by default to see whether a memory corruption occurs
>> during the suspend/resume (it's harmless, but warnings are shown
>> in the kernel dmesg logs).
>>
>> We cannot simply mark the these memory as reserved in E820 table
>> because such configuration makes GRUB complain: unable to allocate
>> real mode page. Hence we choose to back up these memories to the
>> place where we reserved on our stack for our S3 resume work.
>> Before jumping to OS wake up vector, we need restore the original
>> content there.
>>
>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>>
>> ---
>>
>> Changes in v2:
>> - new patch "Fix Windows S3 resume failure"
>>
>>  arch/x86/cpu/cpu.c                 |  8 +++++--
>>  arch/x86/include/asm/acpi_s3.h     | 16 +++++++++++++
>>  arch/x86/include/asm/global_data.h |  1 +
>>  arch/x86/lib/acpi_s3.c             | 48 ++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 71 insertions(+), 2 deletions(-)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>

applied to u-boot-x86/next, thanks!

Patch

diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c
index dfe624f..e13786e 100644
--- a/arch/x86/cpu/cpu.c
+++ b/arch/x86/cpu/cpu.c
@@ -278,13 +278,17 @@  int reserve_arch(void)
 	high_table_reserve();
 #endif
 
-#if defined(CONFIG_HAVE_ACPI_RESUME) && defined(CONFIG_HAVE_FSP)
+#ifdef CONFIG_HAVE_ACPI_RESUME
+	acpi_s3_reserve();
+
+#ifdef CONFIG_HAVE_FSP
 	/*
 	 * Save stack address to CMOS so that at next S3 boot,
 	 * we can use it as the stack address for fsp_contiue()
 	 */
 	fsp_save_s3_stack();
-#endif
+#endif /* CONFIG_HAVE_FSP */
+#endif /* CONFIG_HAVE_ACPI_RESUME */
 
 	return 0;
 }
diff --git a/arch/x86/include/asm/acpi_s3.h b/arch/x86/include/asm/acpi_s3.h
index 1ad20f4..86aec0a 100644
--- a/arch/x86/include/asm/acpi_s3.h
+++ b/arch/x86/include/asm/acpi_s3.h
@@ -29,6 +29,9 @@ 
 #define SLP_TYP_S4	6
 #define SLP_TYP_S5	7
 
+/* Memory size reserved for S3 resume */
+#define S3_RESERVE_SIZE	0x1000
+
 #ifndef __ASSEMBLY__
 
 extern char __wakeup[];
@@ -110,6 +113,19 @@  struct acpi_fadt;
  */
 void acpi_resume(struct acpi_fadt *fadt);
 
+/**
+ * acpi_s3_reserve() - Reserve memory for ACPI S3 resume
+ *
+ * This copies memory where real mode interrupt handler stubs reside to the
+ * reserved place on the stack.
+ *
+ * This routine should be called by reserve_arch() before U-Boot is relocated
+ * when ACPI S3 resume is enabled.
+ *
+ * @return:	0 always
+ */
+int acpi_s3_reserve(void);
+
 #endif /* __ASSEMBLY__ */
 
 #endif /* __ASM_ACPI_S3_H__ */
diff --git a/arch/x86/include/asm/global_data.h b/arch/x86/include/asm/global_data.h
index b4ca473..93a80fe 100644
--- a/arch/x86/include/asm/global_data.h
+++ b/arch/x86/include/asm/global_data.h
@@ -101,6 +101,7 @@  struct arch_global_data {
 #endif
 #ifdef CONFIG_HAVE_ACPI_RESUME
 	int prev_sleep_state;		/* Previous sleep state ACPI_S0/1../5 */
+	ulong backup_mem;		/* Backup memory address for S3 */
 #endif
 };
 
diff --git a/arch/x86/lib/acpi_s3.c b/arch/x86/lib/acpi_s3.c
index 0b62b75..3175da8 100644
--- a/arch/x86/lib/acpi_s3.c
+++ b/arch/x86/lib/acpi_s3.c
@@ -9,6 +9,8 @@ 
 #include <asm/acpi_table.h>
 #include <asm/post.h>
 
+DECLARE_GLOBAL_DATA_PTR;
+
 static void asmlinkage (*acpi_do_wakeup)(void *vector) = (void *)WAKEUP_BASE;
 
 static void acpi_jump_to_wakeup(void *vector)
@@ -29,6 +31,52 @@  void acpi_resume(struct acpi_fadt *fadt)
 
 	wake_vec = acpi_find_wakeup_vector(fadt);
 
+	/*
+	 * Restore the memory content starting from address 0x1000 which is
+	 * used for the real mode interrupt handler stubs.
+	 */
+	memcpy((void *)0x1000, (const void *)gd->arch.backup_mem,
+	       S3_RESERVE_SIZE);
+
 	post_code(POST_OS_RESUME);
 	acpi_jump_to_wakeup(wake_vec);
 }
+
+int acpi_s3_reserve(void)
+{
+	/* adjust stack pointer for ACPI S3 resume backup memory */
+	gd->start_addr_sp -= S3_RESERVE_SIZE;
+	gd->arch.backup_mem = gd->start_addr_sp;
+
+	gd->start_addr_sp &= ~0xf;
+
+	/*
+	 * U-Boot sets up the real mode interrupt handler stubs starting from
+	 * address 0x1000. In most cases, the first 640K (0x00000 - 0x9ffff)
+	 * system memory is reported as system RAM in E820 table to the OS.
+	 * (see install_e820_map() implementation for each platform). So OS
+	 * can use these memories whatever it wants.
+	 *
+	 * If U-Boot is in an S3 resume path, care must be taken not to corrupt
+	 * these memorie otherwise OS data gets lost. Testing shows that, on
+	 * Microsoft Windows 10 on Intel Baytrail its wake up vector happens to
+	 * be installed at the same address 0x1000. While on Linux its wake up
+	 * vector does not overlap this memory range, but after resume kernel
+	 * checks low memory range per config option CONFIG_X86_RESERVE_LOW
+	 * which is 64K by default to see whether a memory corruption occurs
+	 * during the suspend/resume (it's harmless, but warnings are shown
+	 * in the kernel dmesg logs).
+	 *
+	 * We cannot simply mark the these memory as reserved in E820 table
+	 * because such configuration makes GRUB complain: unable to allocate
+	 * real mode page. Hence we choose to back up these memories to the
+	 * place where we reserved on our stack for our S3 resume work.
+	 * Before jumping to OS wake up vector, we need restore the original
+	 * content there (see acpi_resume() above).
+	 */
+	if (gd->arch.prev_sleep_state == ACPI_S3)
+		memcpy((void *)gd->arch.backup_mem, (const void *)0x1000,
+		       S3_RESERVE_SIZE);
+
+	return 0;
+}