x86: mcheck: suspend/resume bug fixes
cover x86: mcheck: suspend/resume bug fixes
Following two patches fix suspend/resume issues with current kernel. Patches are against v2.6.28-rc8-1-g6c34bc2
IMHO this should go into .28.
Thanks,
Andreas
PATCH 1/2 x86: don't enable MCE after suspend/resume when system was booted with "nomce"
Impact: fix suspend/resume bug with MCE
A suspend/resume cycle unconditionally enables MCE
for the boot CPU if MCE is compiled into the kernel.
I.e. even a system booted with "nomce" configures MCE for the boot CPU.
This patch ensures that MCE is not turned on for systems booted with "nomce".
--- arch/x86/kernel/cpu/mcheck/mce_64.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
arch/x86/kernel/cpu/mcheck/mce_64.c +3 -0
@@ -443,6 +443,9 @@ static void mce_init(void *dummy)u64 cap;int i; +if (mce_dont_init) +return ; +rdmsrl (MSR_IA32_MCG_CAP , cap); banks = cap &0xff ;if (banks >MCE_EXTENDED_BANK ) {
Signature · 1 line
1.6.0.4
Re: [PATCH 1/2] x86: don't enable MCE after suspend/resume when system was booted with "nomce"
Quoted 8 lines · Andreas Herrmann
Impact: fix suspend/resume bug with MCE
A suspend/resume cycle unconditionally enables MCE
for the boot CPU if MCE is compiled into the kernel.
I.e. even a system booted with "nomce" configures MCE for the boot CPU.This patch ensures that MCE is not turned on for systems booted with "nomce".
I have that fixed in a better way in
I believe
-Andi
Signature · 1 line
[email protected]
Re: [PATCH 1/2] x86: don't enable MCE after suspend/resume when system was booted with "nomce"
Quoted 8 lines · Andi Kleen
Quoted 6 lines · Andreas Herrmann
Impact: fix suspend/resume bug with MCE
A suspend/resume cycle unconditionally enables MCE
for the boot CPU if MCE is compiled into the kernel.
I.e. even a system booted with "nomce" configures MCE for the boot CPU.
Quoted 6 lines
Quoted 2 lines
This patch ensures that MCE is not turned on for systems booted with "nomce".
I have that fixed in a better way in
Thanks for that pointer.
I didn't look at tip/x86/mce up to know. Is this .28 material? My fix is intended for .28.
I didn't care about the lurking sysfs interface when booted with "nomce". It's there and you could enable MCE with changing attributes but then you are actively fiddling with MCE.
The fix is for the more severe problem that mce_init() is called on resume and thus will enable MCE (setup cr4 and corresponding MSRs) which shouldn't happen when booted with "nomce".
Regards,
Andreas
Re: [PATCH 1/2] x86: don't enable MCE after suspend/resume when system was booted with "nomce"
Quoted 4 lines
Quoted 1 line
Thanks for that pointer.
I didn't look at tip/x86/mce up to know. Is this .28 material?
I would think so, but I'm not sure the x86 maintainers think the same.
Quoted 9 lines
My fix is intended for .28.
I didn't care about the lurking sysfs interface when booted with "nomce". It's there and you could enable MCE with changing attributes but then you are actively fiddling with MCE.
The fix is for the more severe problem that
mce_init()is called on resume and thus will enable MCE (setup cr4 and corresponding MSRs) which shouldn't happen when booted with "nomce".
The patch does the same (although the description doesn't say that). When there is no sysfs interface there won't
be any resume either because resume goes through the sysdev devices. So it's a cleaner and more complete fix imho.
-Andi
PATCH 2/2 x86: re-enable MCE on secondary CPUS after suspend/resume
In reply to Andreas HerrmannImpact: fix suspend/resume bug with MCE
After suspend/resume MCx_CTL registers of secondary CPUs are cleared. (At least that's what I've observed on several systems.)
Linux currently only re-initializes MCE on the boot CPU - see mce_resume(). Thus after suspend/resume we end up with a system where MCE is active on the boot CPU but switched off on all other CPUs.
By calling mce_init() whenever a CPU comes online this problem is solved.
--- arch/x86/kernel/cpu/mcheck/mce_64.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
arch/x86/kernel/cpu/mcheck/mce_64.c +1 -0
@@ -889,6 +889,7 @@ static int __cpuinit mce_cpu_callback(struct notifier_block *nfb,mce_create_device (cpu);if (threshold_cpu_callback)threshold_cpu_callback (action, cpu); +mce_init (NULL );break ;case CPU_DEAD :case CPU_DEAD_FROZEN :
Signature · 1 line
1.6.0.4
Re: [PATCH 2/2] x86: re-enable MCE on secondary CPUS after suspend/resume
Quoted 10 lines · Andreas Herrmann
Impact: fix suspend/resume bug with MCE
After suspend/resume
MCx_CTLregisters of secondary CPUs are cleared. (At least that's what I've observed on several systems.)
Linux currently only re-initializes MCE on the boot CPU - seemce_resume(). Thus after suspend/resume we end up with a system where MCE is active on the boot CPU but switched off on all other CPUs.By calling
mce_init()whenever a CPU comes online this problem is solved.
Can you double check that please?
Suspend/resume are supposted to hotunplug all CPUs except the BP and then re-online them on resume (with "disable_nonboot_cpus()) . The re-online initializes MCEs in the standard CPU bootup path.
A good way is to stick a WARN_ON(num_online_cpus() > 1) into mce_suspend(). I had that here for some time and didn't see it trigger.
I got a couple of suspend bug fixes in my mce improvement tree, see:
During review it was decided to all defer it to .29 though.
-Andi
Signature · 1 line
[email protected]
Re: [PATCH 2/2] x86: re-enable MCE on secondary CPUS after suspend/resume
Quoted 10 lines · Andi Kleen
Quoted 8 lines · Andreas Herrmann
Impact: fix suspend/resume bug with MCE
After suspend/resume
MCx_CTLregisters of secondary CPUs are cleared. (At least that's what I've observed on several systems.)
Linux currently only re-initializes MCE on the boot CPU - seemce_resume(). Thus after suspend/resume we end up with a system where MCE is active on the boot CPU but switched off on all other CPUs.
Quoted 8 lines
Quoted 2 lines
By calling
mce_init()whenever a CPU comes online this problem is solved.Can you double check that please?
Suspend/resume are supposted to hotunplug all CPUs except the BP and then re-online them on resume (with "
disable_nonboot_cpus()) . The re-online initializes MCEs in the standard CPU bootup path.
For BP we have
/* On resume clear all MCE state. Don't want to see leftovers from the BIOS.
Only one CPU is active at this time, the others get readded later using CPU hotplug. */
static int mce_resume(struct sys_device *dev)
{
mce_init(NULL);
return 0;
}For APs mcheck_init() is called on resume. But as the respective bit for an AP is usually set in "mce_cpus" after boot (which is correct, I think) mcheck_init does not call mce_init, see:
void __cpuinit mcheck_init(struct cpuinfo_x86 *c)
{
static cpumask_t mce_cpus = CPU_MASK_NONE;
mce_cpu_quirks(c);
if (mce_dont_init ||
cpu_test_and_set(smp_processor_id(), mce_cpus) ||
!mce_available(c))=> return;
mce_init(NULL);
mce_cpu_features(c);
}But we need to call mce_init to clear all MCE state.
IMHO the best location to call mce_init for APs is the cpu notifier.
Regards,
Andreas
Re: [PATCH 2/2] x86: re-enable MCE on secondary CPUS after suspend/resume
Quoted 9 lines
void
__cpuinitmcheck_init(struct cpuinfo_x86 *c){ static cpumask_t mce_cpus = CPU_MASK_NONE; mce_cpu_quirks(c); if (mce_dont_init || cpu_test_and_set(smp_processor_id(), mce_cpus) || !mce_available(c))
Quoted 8 lines
=> return;
mce_init(NULL); mce_cpu_features(c); }But we need to call
mce_initto clear all MCE state.
IMHO the best location to callmce_initfor APs is the cpu notifier.
Ah got it. Thanks that makes sense.
But I think the better fix is to just drop the mce_cpus check and then handly it naturally in the standard bootup path. I'm not sure what it was good for anyways. I copied it into the 64bit code from 32bit, but I suppose even there it isn't really needed and on 32bit it is already gone even.
How about this patch. Does it fix the problem for you too?
-Andi
--
Don't prevent multiple initialization of MCEs.
Back from early prehistory mcheck_init() has a reentry check. Presumably that was needed in very old kernels to prevent it entering twice.
But as Andreas points out this prevents CPU hotplug (and therefore resume) to correctly reinitialize MCEs when a AP boots again after being offlined.
Just drop the check.
Based on a report from Andreas Herrmann
--- arch/x86/kernel/cpu/mcheck/mce_64.c | 3 --- 1 file changed, 3 deletions(-)
arch/x86/kernel/cpu/mcheck/mce_64.c +0 -3
@@ -510,12 +510,9 @@*/ void __cpuinit mcheck_init (struct cpuinfo_x86 *c) { -static cpumask_t mce_cpus =CPU_MASK_NONE ; -mce_cpu_quirks (c);if (mce_dont_init || -cpu_test_and_set (smp_processor_id (), mce_cpus) || !mce_available (c))return ;
Andreas Herrmann
Tested-byRe: [PATCH 2/2] x86: re-enable MCE on secondary CPUS after suspend/resume
Quoted 20 lines · Andi Kleen
Quoted 9 lines
void
__cpuinitmcheck_init(struct cpuinfo_x86 *c){ static cpumask_t mce_cpus = CPU_MASK_NONE; mce_cpu_quirks(c); if (mce_dont_init || cpu_test_and_set(smp_processor_id(), mce_cpus) || !mce_available(c))Quoted 8 lines
=> return;
mce_init(NULL); mce_cpu_features(c); }But we need to call
mce_initto clear all MCE state.
IMHO the best location to callmce_initfor APs is the cpu notifier.Ah got it. Thanks that makes sense.
Quoted 7 lines
But I think the better fix is to just drop the
mce_cpuscheck and then handly it naturally in the standard bootup path. I'm not sure what it was good for anyways. I copied it into the 64bit code from 32bit, but I suppose even there it isn't really needed and on 32bit it is already gone even.How about this patch. Does it fix the problem for you too?
Yup, works as well:
Thanks,
Andreas
Quoted 42 lines
-Andi
--
Don't prevent multiple initialization of MCEs.
Back from early prehistory
mcheck_init()has a reentry check. Presumably that was needed in very old kernels to prevent it entering twice.But as Andreas points out this prevents CPU hotplug (and therefore resume) to correctly reinitialize MCEs when a AP boots again after being offlined.
Just drop the check.
Based on a report from Andreas Herrmann
Signed-off-by Andi Kleen <[email protected]>--- arch/x86/kernel/cpu/mcheck/mce_64.c | 3 --- 1 file changed, 3 deletions(-)Index:
linux/arch/x86/kernel/cpu/mcheck/mce_64.c
===================================================================arch/x86/kernel/cpu/mcheck/mce_64.c +0
-3@@ -510,12 +510,9 @@*/ void __cpuinit mcheck_init (struct cpuinfo_x86 *c) { -static cpumask_t mce_cpus =CPU_MASK_NONE ; -mce_cpu_quirks (c);if (mce_dont_init || -cpu_test_and_set (smp_processor_id (), mce_cpus) || !mce_available (c))return ;
Ingo Molnar
AppliedRe: [PATCH 2/2] x86: re-enable MCE on secondary CPUS after suspend/resume
Quoted 27 lines · Andreas Herrmann
Quoted 26 lines · Andi Kleen
Quoted 9 lines
void
__cpuinitmcheck_init(struct cpuinfo_x86 *c){ static cpumask_t mce_cpus = CPU_MASK_NONE; mce_cpu_quirks(c); if (mce_dont_init || cpu_test_and_set(smp_processor_id(), mce_cpus) || !mce_available(c))Quoted 8 lines
=> return;
mce_init(NULL); mce_cpu_features(c); }But we need to call
mce_initto clear all MCE state.
IMHO the best location to callmce_initfor APs is the cpu notifier.Ah got it. Thanks that makes sense.
But I think the better fix is to just drop the
mce_cpuscheck and then handly it naturally in the standard bootup path. I'm not sure what it was good for anyways. I copied it into the 64bit code from 32bit, but I suppose even there it isn't really needed and on 32bit it is already gone even.
Quoted 5 lines
Quoted 1 line
How about this patch. Does it fix the problem for you too?
Yup, works as well:
Tested-by Andreas Herrmann <[email protected]>
applied to tip/x86/urgent, thanks guys.
Ingo