From: Fenghua Yu <fenghua.yu@intel.com>
To: "Thomas Gleixner" <tglx@linutronix.de>,
"Ingo Molnar" <mingo@redhat.com>,
"Borislav Petkov" <bp@alien8.de>,
"Valdis Kletnieks" <valdis.kletnieks@vt.edu>,
"x86" <x86@kernel.org>,
"linux-kernel" <linux-kernel@vger.kernel.org>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Subject: [PATCH] x86/umwait: Fix error handling in umwait_init()
Date: Fri, 9 Aug 2019 18:40:37 -0700 [thread overview]
Message-ID: <1565401237-60936-1-git-send-email-fenghua.yu@intel.com> (raw)
Currently, failure of cpuhp_setup_state() is ignored and the syscore
ops and the control interfaces can still be added even after the
failure. But, this error handling will cause a few issues:
1. The CPUs may have different values in the IA32_UMWAIT_CONTROL
MSR because there is no way to roll back the control MSR on
the CPUs which already set the MSR before the failure.
2. If the sysfs interface is added successfully, there will be a mismatch
between the global control value and the control MSR:
- The interface shows the default global control value. But,
the control MSR is not set to the value because the CPU online
function, which is supposed to set the MSR to the value,
is not installed.
- If the sysadmin changes the global control value through
the interface, the control MSR on all current online CPUs is
set to the new value. But, the control MSR on newly onlined CPUs
after the value change will not be set to the new value due to
lack of the CPU online function.
3. On resume from suspend/hibernation, the boot CPU restores the control
MSR to the global control value through the syscore ops. But, the
control MSR on all APs is not set due to lake of the CPU online
function.
To solve the issues and enforce consistent behavior on the failure
of the CPU hotplug setup, make the following changes:
1. Cache the original control MSR value which is configured by
hardware or BIOS before kernel boot. This value is likely to
be 0. But it could be a different number as well. Cache the
control MSR only once before the MSR is changed.
2. Add the CPU offline function so that the MSR is restored to the
original control value on all CPUs on the failure.
3. On the failure, exit from cpumait_init() so that the syscore ops
and the control interfaces are not added.
Reported-by: Valdis Kletnieks <valdis.kletnieks@vt.edu>
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
arch/x86/kernel/cpu/umwait.c | 39 +++++++++++++++++++++++++++++++++++-
1 file changed, 38 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kernel/cpu/umwait.c b/arch/x86/kernel/cpu/umwait.c
index 6a204e7336c1..95581f4cf6d8 100644
--- a/arch/x86/kernel/cpu/umwait.c
+++ b/arch/x86/kernel/cpu/umwait.c
@@ -17,6 +17,12 @@
*/
static u32 umwait_control_cached = UMWAIT_CTRL_VAL(100000, UMWAIT_C02_ENABLE);
+/*
+ * Cache the original IA32_UMWAIT_CONTROL MSR value which is configured by
+ * hardware or BIOS before kernel boot.
+ */
+static u32 orig_umwait_control_cached __read_mostly;
+
/*
* Serialize access to umwait_control_cached and IA32_UMWAIT_CONTROL MSR in
* the sysfs write functions.
@@ -52,6 +58,23 @@ static int umwait_cpu_online(unsigned int cpu)
return 0;
}
+/*
+ * The CPU hotplug callback sets the control MSR to the original control
+ * value.
+ */
+static int umwait_cpu_offline(unsigned int cpu)
+{
+ /*
+ * This code is protected by the CPU hotplug already and
+ * orig_umwait_control_cached is never changed after it caches
+ * the original control MSR value in umwait_init(). So there
+ * is no race condition here.
+ */
+ wrmsr(MSR_IA32_UMWAIT_CONTROL, orig_umwait_control_cached, 0);
+
+ return 0;
+}
+
/*
* On resume, restore IA32_UMWAIT_CONTROL MSR on the boot processor which
* is the only active CPU at this time. The MSR is set up on the APs via the
@@ -185,8 +208,22 @@ static int __init umwait_init(void)
if (!boot_cpu_has(X86_FEATURE_WAITPKG))
return -ENODEV;
+ /*
+ * Cache the original control MSR value before the control MSR is
+ * changed. This is the only place where orig_umwait_control_cached
+ * is modified.
+ */
+ rdmsrl(MSR_IA32_UMWAIT_CONTROL, orig_umwait_control_cached);
+
ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "umwait:online",
- umwait_cpu_online, NULL);
+ umwait_cpu_online, umwait_cpu_offline);
+ if (ret < 0) {
+ /*
+ * On failure, the control MSR on all CPUs has the
+ * original control value.
+ */
+ return ret;
+ }
register_syscore_ops(&umwait_syscore_ops);
--
2.19.1
next reply other threads:[~2019-08-10 1:55 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-10 1:40 Fenghua Yu [this message]
2019-08-10 5:25 ` [PATCH] x86/umwait: Fix error handling in umwait_init() Thomas Gleixner
2019-08-10 13:09 ` Thomas Gleixner
2019-08-12 12:55 ` [tip:x86/urgent] " tip-bot for Fenghua Yu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1565401237-60936-1-git-send-email-fenghua.yu@intel.com \
--to=fenghua.yu@intel.com \
--cc=bp@alien8.de \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=tglx@linutronix.de \
--cc=valdis.kletnieks@vt.edu \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).