linux-kernel - [RFC PATCH] therm_throt: test bits as we build therm_intr_core_clear

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zg3GhhTZotBNvlRR@merlin.infradead.org>
Date: Wed, 3 Apr 2024 17:13:43 -0400
From: Kyle McMartin <kyle@...radead.org>
To: "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>
Cc: Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
	linux-pm@...r.kernel.org, kernel-team@...com,
	linux-kernel@...r.kernel.org
Subject: [RFC PATCH] therm_throt: test bits as we build
 therm_intr_core_clear_mask

X86_FEATURE_HWP appears to be insufficient on some platforms, and
writing 1 to BIT(13)|BIT(15) results in the wrmsrl trapping and failing
to clear PROCHOT_LOG.

Instead, also try to wrmsrl_safe with those bits set in the mask, and
check if we get -EIO back. If so, those bits will trap and prevent us
from writing to THERM_STATUS.

Signed-off-by: Kyle McMartin <jkkm@...com>

---

We noticed a problem on some of our production hosts while rolling out
6.4 kernels where we were seeing PROCHOT_LOG set but never cleared,
along with a warn on once in wrmsr.

I tracked this down to:

commit 930d06bf071aa746db11d68d2d75660b449deff3
Author: Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
Date:   Tue Nov 15 18:54:17 2022 -0800

    thermal: intel: Protect clearing of thermal status bits

which started setting some unexpected bits in THERM_STATUS on our
platform. Previously, the mask had these bits set, but we were masking
with the MSR which was resulting in them not being written to 1.

Starting with 117e4e5bd9d47b89777dbf6b37a709dcfe59520f, these bits were
protected by the HWP CPUID flag, but on some of our platforms, this
doesn't seem sufficient.

On Broadwell and Broadwell-DE, the HWP flag is not set, but writing
these bits does not trap.

On our Skylake-DE, Skylake, and Cooper Lake platforms, the HWP flag is
set in CPUID, and writing 1 to these bits traps attempting to write
0xAAA8 to MSR 0x19C (THERM_STATUS). Writing 0xAA8 from userspace works
as expected to un-stick PROCHOT_LOG.

On our Sapphire Rapids platforms, the HWP flag is set, and writing 1 to
these bits is successful.

 drivers/thermal/intel/therm_throt.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/thermal/intel/therm_throt.c b/drivers/thermal/intel/therm_throt.c
index e69868e868eb..3058d8fcfcef 100644
--- a/drivers/thermal/intel/therm_throt.c
+++ b/drivers/thermal/intel/therm_throt.c
@@ -196,8 +196,14 @@ static const struct attribute_group thermal_attr_group = {
 static u64 therm_intr_core_clear_mask;
 static u64 therm_intr_pkg_clear_mask;

+/* Probe each addition to the mask to ensure that our wrmsrl
+ * won't fail to clear bits.
+ */
 static void thermal_intr_init_core_clear_mask(void)
 {
+	u64 bits = 0;
+	u64 mask = 0;
+
 	if (therm_intr_core_clear_mask)
 		return;

@@ -211,25 +217,34 @@ static void thermal_intr_init_core_clear_mask(void)
 	 * Bit 1, 3, 5: CPUID.01H:EDX[22] = 1. This driver will not
 	 * enable interrupts, when 0 as it checks for X86_FEATURE_ACPI.
 	 */
-	therm_intr_core_clear_mask = (BIT(1) | BIT(3) | BIT(5));
+	mask = (BIT(1) | BIT(3) | BIT(5));

 	/*
 	 * Bit 7 and 9: Thermal Threshold #1 and #2 log
 	 * If CPUID.01H:ECX[8] = 1
 	 */
-	if (boot_cpu_has(X86_FEATURE_TM2))
-		therm_intr_core_clear_mask |= (BIT(7) | BIT(9));
+	bits = BIT(7) | BIT(9);
+	if (boot_cpu_has(X86_FEATURE_TM2) &&
+	    wrmsrl_safe(MSR_IA32_THERM_STATUS, mask | bits) >= 0)
+		mask |= bits;
+

 	/* Bit 11: Power Limitation log (R/WC0) If CPUID.06H:EAX[4] = 1 */
-	if (boot_cpu_has(X86_FEATURE_PLN))
-		therm_intr_core_clear_mask |= BIT(11);
+	bits = BIT(11);
+	if (boot_cpu_has(X86_FEATURE_PLN) &&
+	    wrmsrl_safe(MSR_IA32_THERM_STATUS, mask | bits) >= 0)
+		mask |= bits;

 	/*
 	 * Bit 13: Current Limit log (R/WC0) If CPUID.06H:EAX[7] = 1
 	 * Bit 15: Cross Domain Limit log (R/WC0) If CPUID.06H:EAX[7] = 1
 	 */
-	if (boot_cpu_has(X86_FEATURE_HWP))
-		therm_intr_core_clear_mask |= (BIT(13) | BIT(15));
+	bits = BIT(13) | BIT(15);
+	if (boot_cpu_has(X86_FEATURE_HWP) &&
+	    wrmsrl_safe(MSR_IA32_THERM_STATUS, mask | bits) >= 0)
+		mask |= bits;
+
+	therm_intr_core_clear_mask = mask;
 }

 static void thermal_intr_init_pkg_clear_mask(void)