x86, hotplug: Use hlt instead of mwait when resuming from hibernation

frozen_cpus might be NULL if the allocation in previous alloc_frozen_cpus failed, when CONFIG_CPUMASK_OFFSTACK is set.

This patch avoid accessing this cpumask if it is NULL.

Signed-off-by Chen Yu <[email protected]>

---
 kernel/cpu.c | 4 ++++
 1 file changed, 4 insertions(+)

kernel/cpu.c +0 -0

index d948e44..d25266e 100644

-----Original Message-----
From: Pavel Machek [mailto:[email protected]]
Sent: Sunday, June 26, 2016 12:51 AM
To: Chen, Yu C
Cc: [email protected]; [email protected]; Rafael J. Wysocki; Len Brown; Peter Zijlstra; H. Peter Anvin; Borislav Petkov; Brian Gerst; Thomas Gleixner; Ingo Molnar; Varun Koyyalagunta; [email protected]
Subject: Re: [PATCH 1/4][RFC v2] PM / sleep: Avoid accessing frozen_cpus if it is NULL

Quoted 16 lines · Chen Yu

frozen_cpus might be NULL if the allocation in previous alloc_frozen_cpus failed, when CONFIG_CPUMASK_OFFSTACK is set.

This patch avoid accessing this cpumask if it is NULL.

Signed-off-by Chen Yu <[email protected]>

---
 kernel/cpu.c | 4 ++++
 1 file changed, 4 insertions(+)

kernel/cpu.c +0 -0

@@ -1021,6 +1021,8 @@ int disable_nonboot_cpus(void)  {
 	int cpu, first_cpu, error = 0;
 

kernel/cpu.c int disable_nonboot_cpus(void) · line 1028

Quoted 5 lines

kernel/cpu.c +2 -0

+	if (frozen_cpus == NULL)
+		return -ENOMEM;
 	cpu_maps_update_begin();
 	first_cpu = cpumask_first(cpu_online_mask);
 	/*

I'd say that whoever allocates frozen_cpus should just abort the hibernation if there's not enough memory for the operation...? This seems like checking for the problem too late.

-----Original Message-----
From: Chen, Yu C
Sent: Sunday, June 26, 2016 9:17 AM
To: 'Pavel Machek'
Cc: [email protected]; [email protected]; Rafael J. Wysocki; Len Brown; Peter Zijlstra; H. Peter Anvin; Borislav Petkov; Brian Gerst; Thomas Gleixner; Ingo Molnar; Varun Koyyalagunta; [email protected]
Subject: RE: [PATCH 1/4][RFC v2] PM / sleep: Avoid accessing frozen_cpus if it is NULL

Hi,

Quoted 11 lines

-----Original Message-----
From: Pavel Machek [mailto:[email protected]]
Sent: Sunday, June 26, 2016 12:51 AM
To: Chen, Yu C
Cc: [email protected]; [email protected]; Rafael J. Wysocki; Len Brown; Peter Zijlstra; H. Peter Anvin; Borislav Petkov; Brian Gerst; Thomas Gleixner; Ingo Molnar; Varun Koyyalagunta; [email protected]
Subject: Re: [PATCH 1/4][RFC v2] PM / sleep: Avoid accessing frozen_cpus if it is NULL

Quoted 5 lines

I'd say that whoever allocates frozen_cpus should just abort the hibernation if there's not enough memory for the operation...? This seems like checking for the problem too late.

The allocation of frozen_cpus is alloc_frozen_cpus, which is in core_initcall, and do_initcall during boot up seems not to care about the return value from these functions. So I think either we add the check in disable_nonboot_cpus, or we can set noresume = 1 and nohibernate = 1 if alloc_frozen_cpus fails.

There is requirement that we need to do some arch-specific operations before putting the nonboot CPUs offline/online. One of the requirements comes from the hibernation resume process on x86_64, we need to kick all the offlin-CPUs online and offline again, in order to put them in a safe state, thus to avoid possible unwilling wake up during hibernation resume.

-----Original Message-----
From: Pavel Machek [mailto:[email protected]]
Sent: Sunday, June 26, 2016 12:52 AM
To: Chen, Yu C
Cc: [email protected]; [email protected]; Rafael J. Wysocki; Len Brown; Peter Zijlstra; H. Peter Anvin; Borislav Petkov; Brian Gerst; Thomas Gleixner; Ingo Molnar; Varun Koyyalagunta; [email protected]
Subject: Re: [PATCH 2/4][RFC v2] PM / sleep: Introduce arch-specific hook for disable/enable nonboot cpus

On Sun 2016-06-26 00:18:41, Chen Yu wrote:

Quoted 6 lines

There is requirement that we need to do some arch-specific operations before putting the nonboot CPUs offline/online.
One of the requirements comes from the hibernation resume process on x86_64, we need to kick all the offlin-CPUs online and offline again, in order to put them in a safe state, thus to avoid possible unwilling wake up during hibernation resume.

You may want to check english here.

There is requirement that we need to do some arch-specific operations before putting the nonboot CPUs offline/online. One of the requirements comes from the hibernation resume process on x86_64, we need to kick all the offlin-CPUs online and offline again, in order to put them in a safe state, thus to avoid possible unwilling wake up during hibernation resume.

Signed-off-by Chen Yu <[email protected]>

---
 kernel/cpu.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

kernel/cpu.c +0 -0

index d25266e..ce6e5e4 100644

Quoted 18 lines · Chen Yu

There is requirement that we need to do some arch-specific operations before putting the nonboot CPUs offline/online. One of the requirements comes from the hibernation resume process on x86_64, we need to kick all the offlin-CPUs online and offline again, in order to put them in a safe state, thus to avoid possible unwilling wake up during hibernation resume.

Signed-off-by Chen Yu <[email protected]>

---
kernel/cpu.c | 12 ++++++++++++

1 file changed, 12 insertions(+)

kernel/cpu.c +0 -0

index d25266e..ce6e5e4 100644

kernel/cpu.c EXPORT_SYMBOL_GPL(cpu_up); · line 1020

Quoted 4 lines

kernel/cpu.c +1 -0

@@ -1017,6 +1017,16 @@ EXPORT_SYMBOL_GPL(cpu_up);
 #ifdef CONFIG_PM_SLEEP_SMP
 static cpumask_var_t frozen_cpus;
 
+void __weak arch_disable_nonboot_cpus_pre(void)

I don't like using __weak. It penalizes code size on architectures that don't hook these functions. My preferred pattern is:

include/linux/something.h:

#include <asm/something.h>

#ifndef arch_do_xyz
static inline void arch_do_xyz() {}
#endif

arch/whatever/asm/something.h:

extern void arch_do_xyz(); /* or static inline... */ #define arch_do_xyz

This is totally free for architectures that don't have the hooks and it can potentially be inlined on architectures that do have the hooks. Everyone wins except that it's about five additional lines of code.

--Andy

Quoted 18 lines · Chen Yu

There is requirement that we need to do some arch-specific operations before putting the nonboot CPUs offline/online. One of the requirements comes from the hibernation resume process on x86_64, we need to kick all the offlin-CPUs online and offline again, in order to put them in a safe state, thus to avoid possible unwilling wake up during hibernation resume.

Signed-off-by Chen Yu <[email protected]>

---
kernel/cpu.c | 12 ++++++++++++

1 file changed, 12 insertions(+)

kernel/cpu.c +0 -0

index d25266e..ce6e5e4 100644

kernel/cpu.c EXPORT_SYMBOL_GPL(cpu_up); · line 1020

Quoted 4 lines

kernel/cpu.c +1 -0

@@ -1017,6 +1017,16 @@ EXPORT_SYMBOL_GPL(cpu_up);
 #ifdef CONFIG_PM_SLEEP_SMP
 static cpumask_var_t frozen_cpus;
 
+void __weak arch_disable_nonboot_cpus_pre(void)

I don't like using __weak. It penalizes code size on architectures that don't hook these functions. My preferred pattern is:

include/linux/something.h:

#include <asm/something.h>

#ifndef arch_do_xyz
static inline void arch_do_xyz() {}
#endif

arch/whatever/asm/something.h:

extern void arch_do_xyz(); /* or static inline... */ #define arch_do_xyz

This is totally free for architectures that don't have the hooks and it can potentially be inlined on architectures that do have the hooks. Everyone wins except that it's about five additional lines of code.

Sometime we need to do some operations before resuming from hibernation, so introduce a flag to indicate this stage.

Signed-off-by Chen Yu <[email protected]>

---
 include/linux/suspend.h  | 7 +++++++
 kernel/power/hibernate.c | 3 +++
 2 files changed, 10 insertions(+)

include/linux/suspend.h +0 -0

index 8b6ec7e..422e87a 100644

include/linux/suspend.h +5 -0

+static inline bool hibernation_in_resume(void)
+{
+	return in_resume_hibernate;
+}
 #else /* CONFIG_HIBERNATION */
 static inline void register_nosave_region(unsigned long b, unsigned long e) {}
 static inline void register_nosave_region_late(unsigned long b, unsigned long e) {}

@@ -395,6 +401,7 @@ static inline void hibernation_set_ops(const struct platform_hibernation_ops *op
 static inline int hibernate(void) { return -ENOSYS; }
 static inline bool system_entering_hibernation(void) { return false; }
 static inline bool hibernation_available(void) { return false; }
+static inline bool hibernation_in_resume(void) { return false; }
 #endif /* CONFIG_HIBERNATION */
 
 /* Hibernation and suspend events */

kernel/power/hibernate.c +3 -0

index fca9254..41be909 100644
@@ -43,6 +43,7 @@ static char resume_file[256] = CONFIG_PM_STD_PARTITION;
 dev_t swsusp_resume_device;
 sector_t swsusp_resume_block;
 __visible int in_suspend __nosavedata;
+bool in_resume_hibernate;
 
 enum {
 	HIBERNATION_INVALID,

@@ -433,6 +434,7 @@ static int resume_target_kernel(bool platform_mode)
 	if (error)
 		goto Cleanup;
 
+	in_resume_hibernate = true;
 	error = disable_nonboot_cpus();
 	if (error)
 		goto Enable_cpus;

@@ -474,6 +476,7 @@ static int resume_target_kernel(bool platform_mode)
 	local_irq_enable();
 
  Enable_cpus:
+	in_resume_hibernate = false;
 	enable_nonboot_cpus();
 
  Cleanup:

-----Original Message-----
From: Pavel Machek [mailto:[email protected]]
Sent: Sunday, June 26, 2016 12:53 AM
To: Chen, Yu C
Cc: [email protected]; [email protected]; Rafael J. Wysocki; Len Brown; Peter Zijlstra; H. Peter Anvin; Borislav Petkov; Brian Gerst; Thomas Gleixner; Ingo Molnar; Varun Koyyalagunta; [email protected]
Subject: Re: [PATCH 3/4][RFC v2] PM / hibernate: introduce a flag to indicate resuming from hibernation

Quoted 9 lines · Chen Yu

Sometime we need to do some operations before resuming from hibernation, so introduce a flag to indicate this stage.

Signed-off-by Chen Yu <[email protected]>

---
 include/linux/suspend.h  | 7 +++++++
 kernel/power/hibernate.c | 3 +++
 2 files changed, 10 insertions(+)

include/linux/suspend.h extern bool system_entering_hibernation(void); · line 386

Quoted 8 lines

include/linux/suspend.h index renamed from include/linux/suspend.h +0 -0

8b6ec7e..422e87a 100644

include/linux/suspend.h +1 -0

@@ -384,6 +384,12 @@ extern bool system_entering_hibernation(void);
 extern bool hibernation_available(void);  asmlinkage int
 swsusp_save(void);  extern struct pbe *restore_pblist;
+extern bool in_resume_hibernate;

in_resume_hibernation? But it is pretty sad if we need another such state function...

Here's the story of what the problem is, why this
happened, and why this patch looks like this:

Stress test from Varun Koyyalagunta reports that, the nonboot CPU would hang occasionally, when resuming from hibernation. Further investigation shows that, the precise stage when nonboot CPU hangs, is the time when the nonboot CPU been woken up incorrectly, and tries to monitor the mwait_ptr for the second time, then an exception is triggered due to illegal vaddr access, say, something like, 'Unable to handler kernel address of 0xffff8800ba800010...'

Further investigation shows that, the exception is caused by accessing a page without PRESENT flag, because the pte entry for this vaddr is zero. Here's the scenario how this problem
happens: Page table for direct mapping is allocated dynamically by kernel_physical_mapping_init, it is possible that in the resume process, when the boot CPU is trying to write back pages to their original address, and just right to writes to the monitor mwait_ptr then wakes up one of the nonboot CPUs, since the page table currently used by the nonboot CPU might not the same as it is before the hibernation, an exception might occur due to inconsistent page table.

First try is to get rid of this problem by changing the monitor address from task.flag to zero page, because one one would write to zero page. But this still have problem because of ping-pong wake up situation in mwait_play_dead:

One possible implementation of a clflush is a read-invalidate snoop, which is what a store might look like, so cflush might break the mwait.

1. CPU1 wait at zero page
2. CPU2 cflush zero page, wake CPU1 up, then CPU2 waits at zero page
3. CPU1 is woken up, and invoke cflush zero page, thus wake up CPU2 again. then the nonboot CPUs never sleep for long.

So it's better to monitor different address for each nonboot CPUs, however since there is only one zero page, at most:
PAGE_SIZE/L1_CACHE_LINE CPUs are satisfied, which is usually 64 on a x86_64, apparently it's not enough for servers, maybe more zero pages are required.

So choose the solution as Brian suggested, to put the nonboot CPUs into hlt before resuming. But Rafael has mentioned that, if some of the CPUs have already been offline before hibernation, then the problem is still there. So this patch tries to kick the already offline CPUs woken up and fall into hlt, and then put the rest online CPUs into hlt. In this way, all the nonboot CPUs will wait at a safe state, without touching any memory during s/r. (It's not safe to modify mwait_play_dead, because once previous offline CPUs are woken up, it will either access text code, whose page table is not safe anymore across hibernation, due to:
Commit ab76f7b4ab23 ("x86/mm: Set NX on gap between __ex_table and rodata").

Quoted 44 lines · Chen Yu

Here's the story of what the problem is, why this
happened, and why this patch looks like this:

Stress test from Varun Koyyalagunta reports that, the nonboot CPU would hang occasionally, when resuming from hibernation. Further investigation shows that, the precise stage when nonboot CPU hangs, is the time when the nonboot CPU been woken up incorrectly, and tries to monitor the mwait_ptr for the second time, then an exception is triggered due to illegal vaddr access, say, something like, 'Unable to handler kernel address of 0xffff8800ba800010...'

Further investigation shows that, the exception is caused by accessing a page without PRESENT flag, because the pte entry for this vaddr is zero. Here's the scenario how this problem
happens: Page table for direct mapping is allocated dynamically by kernel_physical_mapping_init, it is possible that in the resume process, when the boot CPU is trying to write back pages to their original address, and just right to writes to the monitor mwait_ptr then wakes up one of the nonboot CPUs, since the page table currently used by the nonboot CPU might not the same as it is before the hibernation, an exception might occur due to inconsistent page table.

First try is to get rid of this problem by changing the monitor address from task.flag to zero page, because one one would write to zero page. But this still have problem because of ping-pong wake up situation in mwait_play_dead:

One possible implementation of a clflush is a read-invalidate snoop, which is what a store might look like, so cflush might break the mwait.

1. CPU1 wait at zero page
2. CPU2 cflush zero page, wake CPU1 up, then CPU2 waits at zero page
3. CPU1 is woken up, and invoke cflush zero page, thus wake up CPU2 again. then the nonboot CPUs never sleep for long.

So it's better to monitor different address for each nonboot CPUs, however since there is only one zero page, at most:
PAGE_SIZE/L1_CACHE_LINE CPUs are satisfied, which is usually 64 on a x86_64, apparently it's not enough for servers, maybe more zero pages are required.

Quoted 13 lines

So choose the solution as Brian suggested, to put the nonboot CPUs into hlt before resuming. But Rafael has mentioned that, if some of the CPUs have already been offline before hibernation, then the problem is still there. So this patch tries to kick the already offline CPUs woken up and fall into hlt, and then put the rest online CPUs into hlt. In this way, all the nonboot CPUs will wait at a safe state, without touching any memory during s/r. (It's not safe to modify mwait_play_dead, because once previous offline CPUs are woken up, it will either access text code, whose page table is not safe anymore across hibernation, due to:
Commit ab76f7b4ab23 ("x86/mm: Set NX on gap between __ex_table and rodata").

I realize I'm extremely late to the party, but I must admit that I don't get it. Sure, hibernation resume can spuriously wake the non-boot CPU, but at some point it has to wake up for real.

What ensures that the text it was
running (native_play_dead or whatever) is still there when it wakes up?

Or does the hibernation resume code actually send the remote CPU an INIT-SIPI sequence a la wakeup_secondary_cpu_via_init()?

If so, this seems
a bit odd to me. Shouldn't we kick the CPU all the way to the wait-for-SIPI state rather than getting it to play dead via hlt or mwait?

Or perhaps
there should be a real hibernation trampoline: allocate a safe page, have all CPUs jump there and spin (via hlt or mwait), do the resume, and then kick the other CPUs awake and have them jump back to real code.

Quoted 59 lines · Andy Lutomirski

Quoted 44 lines · Chen Yu

Here's the story of what the problem is, why this
happened, and why this patch looks like this:

Stress test from Varun Koyyalagunta reports that, the nonboot CPU would hang occasionally, when resuming from hibernation. Further investigation shows that, the precise stage when nonboot CPU hangs, is the time when the nonboot CPU been woken up incorrectly, and tries to monitor the mwait_ptr for the second time, then an exception is triggered due to illegal vaddr access, say, something like, 'Unable to handler kernel address of 0xffff8800ba800010...'

Further investigation shows that, the exception is caused by accessing a page without PRESENT flag, because the pte entry for this vaddr is zero. Here's the scenario how this problem
happens: Page table for direct mapping is allocated dynamically by kernel_physical_mapping_init, it is possible that in the resume process, when the boot CPU is trying to write back pages to their original address, and just right to writes to the monitor mwait_ptr then wakes up one of the nonboot CPUs, since the page table currently used by the nonboot CPU might not the same as it is before the hibernation, an exception might occur due to inconsistent page table.

First try is to get rid of this problem by changing the monitor address from task.flag to zero page, because one one would write to zero page. But this still have problem because of ping-pong wake up situation in mwait_play_dead:

One possible implementation of a clflush is a read-invalidate snoop, which is what a store might look like, so cflush might break the mwait.

1. CPU1 wait at zero page
2. CPU2 cflush zero page, wake CPU1 up, then CPU2 waits at zero page
3. CPU1 is woken up, and invoke cflush zero page, thus wake up CPU2 again. then the nonboot CPUs never sleep for long.

So it's better to monitor different address for each nonboot CPUs, however since there is only one zero page, at most:
PAGE_SIZE/L1_CACHE_LINE CPUs are satisfied, which is usually 64 on a x86_64, apparently it's not enough for servers, maybe more zero pages are required.

Quoted 13 lines

So choose the solution as Brian suggested, to put the nonboot CPUs into hlt before resuming. But Rafael has mentioned that, if some of the CPUs have already been offline before hibernation, then the problem is still there. So this patch tries to kick the already offline CPUs woken up and fall into hlt, and then put the rest online CPUs into hlt. In this way, all the nonboot CPUs will wait at a safe state, without touching any memory during s/r. (It's not safe to modify mwait_play_dead, because once previous offline CPUs are woken up, it will either access text code, whose page table is not safe anymore across hibernation, due to:
Commit ab76f7b4ab23 ("x86/mm: Set NX on gap between __ex_table and rodata").

Quoted 3 lines

I realize I'm extremely late to the party, but I must admit that I don't get it. Sure, hibernation resume can spuriously wake the non-boot CPU, but at some point it has to wake up for real.

You mean during resume? We reinit from scratch then.

Quoted 5 lines

What ensures that the text it was
running (native_play_dead or whatever) is still there when it wakes up?

Or does the hibernation resume code actually send the remote CPU an INIT-SIPI sequence a la wakeup_secondary_cpu_via_init()?

That's what happens AFAICS.

Quoted 3 lines

If so, this seems
a bit odd to me. Shouldn't we kick the CPU all the way to the wait-for-SIPI state rather than getting it to play dead via hlt or mwait?

We could do that. It would be a bit cleaner than using the "hlt play dead" thing, but the practical difference would be very small (if observable at all).

Quoted 74 lines · Rafael J. Wysocki

Quoted 59 lines · Andy Lutomirski

Quoted 44 lines · Chen Yu

Here's the story of what the problem is, why this
happened, and why this patch looks like this:

Stress test from Varun Koyyalagunta reports that, the nonboot CPU would hang occasionally, when resuming from hibernation. Further investigation shows that, the precise stage when nonboot CPU hangs, is the time when the nonboot CPU been woken up incorrectly, and tries to monitor the mwait_ptr for the second time, then an exception is triggered due to illegal vaddr access, say, something like, 'Unable to handler kernel address of 0xffff8800ba800010...'

Further investigation shows that, the exception is caused by accessing a page without PRESENT flag, because the pte entry for this vaddr is zero. Here's the scenario how this problem
happens: Page table for direct mapping is allocated dynamically by kernel_physical_mapping_init, it is possible that in the resume process, when the boot CPU is trying to write back pages to their original address, and just right to writes to the monitor mwait_ptr then wakes up one of the nonboot CPUs, since the page table currently used by the nonboot CPU might not the same as it is before the hibernation, an exception might occur due to inconsistent page table.

First try is to get rid of this problem by changing the monitor address from task.flag to zero page, because one one would write to zero page. But this still have problem because of ping-pong wake up situation in mwait_play_dead:

One possible implementation of a clflush is a read-invalidate snoop, which is what a store might look like, so cflush might break the mwait.

1. CPU1 wait at zero page
2. CPU2 cflush zero page, wake CPU1 up, then CPU2 waits at zero page
3. CPU1 is woken up, and invoke cflush zero page, thus wake up CPU2 again. then the nonboot CPUs never sleep for long.

So it's better to monitor different address for each nonboot CPUs, however since there is only one zero page, at most:
PAGE_SIZE/L1_CACHE_LINE CPUs are satisfied, which is usually 64 on a x86_64, apparently it's not enough for servers, maybe more zero pages are required.

Quoted 13 lines

So choose the solution as Brian suggested, to put the nonboot CPUs into hlt before resuming. But Rafael has mentioned that, if some of the CPUs have already been offline before hibernation, then the problem is still there. So this patch tries to kick the already offline CPUs woken up and fall into hlt, and then put the rest online CPUs into hlt. In this way, all the nonboot CPUs will wait at a safe state, without touching any memory during s/r. (It's not safe to modify mwait_play_dead, because once previous offline CPUs are woken up, it will either access text code, whose page table is not safe anymore across hibernation, due to:
Commit ab76f7b4ab23 ("x86/mm: Set NX on gap between __ex_table and rodata").

Quoted 3 lines

I realize I'm extremely late to the party, but I must admit that I don't get it. Sure, hibernation resume can spuriously wake the non-boot CPU, but at some point it has to wake up for real.

You mean during resume? We reinit from scratch then.

Quoted 5 lines

What ensures that the text it was
running (native_play_dead or whatever) is still there when it wakes up?

Or does the hibernation resume code actually send the remote CPU an INIT-SIPI sequence a la wakeup_secondary_cpu_via_init()?

That's what happens AFAICS.

Quoted 7 lines

Quoted 3 lines

If so, this seems
a bit odd to me. Shouldn't we kick the CPU all the way to the wait-for-SIPI state rather than getting it to play dead via hlt or mwait?

We could do that. It would be a bit cleaner than using the "hlt play dead" thing, but the practical difference would be very small (if observable at all).

Probably true. It might be worth changing the "hlt" path to something like:

asm volatile ("hlt");
WARN(1, "CPU woke directly from halt-for-resume -- should have been woken by SIPI\n");

Quoted 85 lines · Andy Lutomirski

Quoted 74 lines · Rafael J. Wysocki

Quoted 59 lines · Andy Lutomirski

Quoted 44 lines · Chen Yu

Here's the story of what the problem is, why this
happened, and why this patch looks like this:

Stress test from Varun Koyyalagunta reports that, the nonboot CPU would hang occasionally, when resuming from hibernation. Further investigation shows that, the precise stage when nonboot CPU hangs, is the time when the nonboot CPU been woken up incorrectly, and tries to monitor the mwait_ptr for the second time, then an exception is triggered due to illegal vaddr access, say, something like, 'Unable to handler kernel address of 0xffff8800ba800010...'

Further investigation shows that, the exception is caused by accessing a page without PRESENT flag, because the pte entry for this vaddr is zero. Here's the scenario how this problem
happens: Page table for direct mapping is allocated dynamically by kernel_physical_mapping_init, it is possible that in the resume process, when the boot CPU is trying to write back pages to their original address, and just right to writes to the monitor mwait_ptr then wakes up one of the nonboot CPUs, since the page table currently used by the nonboot CPU might not the same as it is before the hibernation, an exception might occur due to inconsistent page table.

First try is to get rid of this problem by changing the monitor address from task.flag to zero page, because one one would write to zero page. But this still have problem because of ping-pong wake up situation in mwait_play_dead:

One possible implementation of a clflush is a read-invalidate snoop, which is what a store might look like, so cflush might break the mwait.

1. CPU1 wait at zero page
2. CPU2 cflush zero page, wake CPU1 up, then CPU2 waits at zero page
3. CPU1 is woken up, and invoke cflush zero page, thus wake up CPU2 again. then the nonboot CPUs never sleep for long.

So it's better to monitor different address for each nonboot CPUs, however since there is only one zero page, at most:
PAGE_SIZE/L1_CACHE_LINE CPUs are satisfied, which is usually 64 on a x86_64, apparently it's not enough for servers, maybe more zero pages are required.

Quoted 13 lines

So choose the solution as Brian suggested, to put the nonboot CPUs into hlt before resuming. But Rafael has mentioned that, if some of the CPUs have already been offline before hibernation, then the problem is still there. So this patch tries to kick the already offline CPUs woken up and fall into hlt, and then put the rest online CPUs into hlt. In this way, all the nonboot CPUs will wait at a safe state, without touching any memory during s/r. (It's not safe to modify mwait_play_dead, because once previous offline CPUs are woken up, it will either access text code, whose page table is not safe anymore across hibernation, due to:
Commit ab76f7b4ab23 ("x86/mm: Set NX on gap between __ex_table and rodata").

Quoted 3 lines

I realize I'm extremely late to the party, but I must admit that I don't get it. Sure, hibernation resume can spuriously wake the non-boot CPU, but at some point it has to wake up for real.

You mean during resume? We reinit from scratch then.

Quoted 5 lines

What ensures that the text it was
running (native_play_dead or whatever) is still there when it wakes up?

Or does the hibernation resume code actually send the remote CPU an INIT-SIPI sequence a la wakeup_secondary_cpu_via_init()?

That's what happens AFAICS.

Quoted 7 lines

Quoted 3 lines

If so, this seems
a bit odd to me. Shouldn't we kick the CPU all the way to the wait-for-SIPI state rather than getting it to play dead via hlt or mwait?

We could do that. It would be a bit cleaner than using the "hlt play dead" thing, but the practical difference would be very small (if observable at all).

Probably true. It might be worth changing the "hlt" path to something like:

Quoted 3 lines

asm volatile ("hlt");
WARN(1, "CPU woke directly from halt-for-resume -- should have been woken by SIPI\n");

The visibility of that warning would be sort of limited, though, because the only case it might show up is when the system went belly up due to an unhandled page fault.

Quoted 43 lines · Chen Yu

Here's the story of what the problem is, why this
happened, and why this patch looks like this:

Stress test from Varun Koyyalagunta reports that, the nonboot CPU would hang occasionally, when resuming from hibernation. Further investigation shows that, the precise stage when nonboot CPU hangs, is the time when the nonboot CPU been woken up incorrectly, and tries to monitor the mwait_ptr for the second time, then an exception is triggered due to illegal vaddr access, say, something like, 'Unable to handler kernel address of 0xffff8800ba800010...'

Further investigation shows that, the exception is caused by accessing a page without PRESENT flag, because the pte entry for this vaddr is zero. Here's the scenario how this problem
happens: Page table for direct mapping is allocated dynamically by kernel_physical_mapping_init, it is possible that in the resume process, when the boot CPU is trying to write back pages to their original address, and just right to writes to the monitor mwait_ptr then wakes up one of the nonboot CPUs, since the page table currently used by the nonboot CPU might not the same as it is before the hibernation, an exception might occur due to inconsistent page table.

First try is to get rid of this problem by changing the monitor address from task.flag to zero page, because one one would write to zero page. But this still have problem because of ping-pong wake up situation in mwait_play_dead:

One possible implementation of a clflush is a read-invalidate snoop, which is what a store might look like, so cflush might break the mwait.

1. CPU1 wait at zero page
2. CPU2 cflush zero page, wake CPU1 up, then CPU2 waits at zero page
3. CPU1 is woken up, and invoke cflush zero page, thus wake up CPU2 again. then the nonboot CPUs never sleep for long.

So it's better to monitor different address for each nonboot CPUs, however since there is only one zero page, at most:
PAGE_SIZE/L1_CACHE_LINE CPUs are satisfied, which is usually 64 on a x86_64, apparently it's not enough for servers, maybe more zero pages are required.

Quoted 13 lines

So choose the solution as Brian suggested, to put the nonboot CPUs into hlt before resuming. But Rafael has mentioned that, if some of the CPUs have already been offline before hibernation, then the problem is still there. So this patch tries to kick the already offline CPUs woken up and fall into hlt, and then put the rest online CPUs into hlt. In this way, all the nonboot CPUs will wait at a safe state, without touching any memory during s/r. (It's not safe to modify mwait_play_dead, because once previous offline CPUs are woken up, it will either access text code, whose page table is not safe anymore across hibernation, due to:
Commit ab76f7b4ab23 ("x86/mm: Set NX on gap between __ex_table and rodata").

I realize I'm extremely late to the party, but I must admit that I don't get it. Sure, hibernation resume can spuriously wake the non-boot CPU, but at some point it has to wake up for real. What ensures that the text it was running (native_play_dead or whatever) is still there when it wakes up?

--Andy