Change in coreboot[master]: include/cpu/x86: introduce new helper for (un)setting MSRs

[Nico Huber (Code Review)]

Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46354 )

Change subject: include/cpu/x86: introduce new helper for (un)setting MSRs
......................................................................

include/cpu/x86: introduce new helper for (un)setting MSRs

msr_set_bit can only set single bits in MSRs and causes mixing of bit
positions and bitmasks in the MSR header files. Thus, replace the helper
by versions which can unset and set whole MSR bitmasks, just like the
"and-or"-helper, but in the way commit 64a6b6c was done (inversion done
in the helper). This helps keeping the MSR macros unified in bitmask
style.

In sum, the three helpers msr_set, msr_unset and msr_unset_and_set get
added.

The few uses of msr_set_bit have been replaced by the new version, while
the used macros have been converted accordingly.

Change-Id: Idfe9b66e7cfe78ec295a44a2a193f530349f7689
Signed-off-by: Michael Niewöhner 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/46354
Reviewed-by: Nico Huber 
Tested-by: build bot (Jenkins) 
---
M src/cpu/intel/haswell/finalize.c
M src/cpu/intel/model_2065x/finalize.c
M src/cpu/intel/model_206ax/finalize.c
M src/include/cpu/x86/msr.h
M src/soc/intel/common/block/cpu/cpulib.c
M src/soc/intel/common/block/include/intelblocks/msr.h
6 files changed, 48 insertions(+), 22 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved

diff --git a/src/cpu/intel/haswell/finalize.c b/src/cpu/intel/haswell/finalize.c
index 1832e63..3bf9225 100644
--- a/src/cpu/intel/haswell/finalize.c
+++ b/src/cpu/intel/haswell/finalize.c
@@ -1,5 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0-only */

+#include <types.h>
 #include <arch/cpu.h>
 #include <cpu/x86/msr.h>
 #include "haswell.h"
@@ -7,5 +8,5 @@
 void intel_cpu_haswell_finalize_smm(void)
 {
    /* Lock memory configuration to protect SMM */
-	msr_set_bit(MSR_LT_LOCK_MEMORY, 0);
+	msr_set(MSR_LT_LOCK_MEMORY, BIT(0));
 }
diff --git a/src/cpu/intel/model_2065x/finalize.c b/src/cpu/intel/model_2065x/finalize.c
index d530fba..d19ddf7 100644
--- a/src/cpu/intel/model_2065x/finalize.c
+++ b/src/cpu/intel/model_2065x/finalize.c
@@ -1,5 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0-only */

+#include <types.h>
 #include <arch/cpu.h>
 #include <cpu/x86/msr.h>
 #include <cpu/intel/speedstep.h>
@@ -13,12 +14,12 @@
 void intel_model_2065x_finalize_smm(void)
 {
    /* Lock C-State MSR */
-	msr_set_bit(MSR_PKG_CST_CONFIG_CONTROL, 15);
+	msr_set(MSR_PKG_CST_CONFIG_CONTROL, BIT(15));

/* Lock AES-NI only if supported */
    if (cpuid_ecx(1) & (1 << 25))
-		msr_set_bit(MSR_FEATURE_CONFIG, 0);
+		msr_set(MSR_FEATURE_CONFIG, BIT(0));

/* Lock TM interrupts - route thermal events to all processors */
-	msr_set_bit(MSR_MISC_PWR_MGMT, 22);
+	msr_set(MSR_MISC_PWR_MGMT, BIT(22));
 }
diff --git a/src/cpu/intel/model_206ax/finalize.c b/src/cpu/intel/model_206ax/finalize.c
index 37fbefd..98be012 100644
--- a/src/cpu/intel/model_206ax/finalize.c
+++ b/src/cpu/intel/model_206ax/finalize.c
@@ -1,5 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0-only */

+#include <types.h>
 #include <arch/cpu.h>
 #include <cpu/x86/msr.h>
 #include "model_206ax.h"
@@ -13,11 +14,11 @@
 {
    /* Lock AES-NI only if supported */
    if (cpuid_ecx(1) & (1 << 25))
-		msr_set_bit(MSR_FEATURE_CONFIG, 0);
+		msr_set(MSR_FEATURE_CONFIG, BIT(0));

/* Lock TM interrupts - route thermal events to all processors */
-	msr_set_bit(MSR_MISC_PWR_MGMT, 22);
+	msr_set(MSR_MISC_PWR_MGMT, BIT(22));

/* Lock memory configuration to protect SMM */
-	msr_set_bit(MSR_LT_LOCK_MEMORY, 0);
+	msr_set(MSR_LT_LOCK_MEMORY, BIT(0));
 }
diff --git a/src/include/cpu/x86/msr.h b/src/include/cpu/x86/msr.h
index 3deb133..058419f 100644
--- a/src/include/cpu/x86/msr.h
+++ b/src/include/cpu/x86/msr.h
@@ -299,23 +299,46 @@
    return MCA_ERRTYPE_UNKNOWN;
 }

-/* Helper for setting single MSR bits */
-static inline void msr_set_bit(unsigned int reg, unsigned int bit)
+/**
+ * Helper for (un)setting MSR bitmasks
+ *
+ * @param[in] reg	The MSR.
+ * @param[in] unset	Bitmask with ones to the bits to unset from the MSR.
+ * @param[in] set	Bitmask with ones to the bits to set from the MSR.
+ */
+static inline void msr_unset_and_set(unsigned int reg, uint64_t unset, uint64_t set)
 {
-	msr_t msr = rdmsr(reg);
+	msr_t msr;

-	if (bit < 32) {
-		if (msr.lo & (1 << bit))
-			return;
-		msr.lo |= 1 << bit;
-	} else {
-		if (msr.hi & (1 << (bit - 32)))
-			return;
-		msr.hi |= 1 << (bit - 32);
-	}
-
+	msr = rdmsr(reg);
+	msr.lo &= (unsigned int)~unset;
+	msr.hi &= (unsigned int)~(unset >> 32);
+	msr.lo |= (unsigned int)set;
+	msr.hi |= (unsigned int)(set >> 32);
    wrmsr(reg, msr);
 }

+/**
+ * Helper for setting MSR bitmasks
+ *
+ * @param[in] reg	The MSR.
+ * @param[in] set	Bitmask with ones to the bits to set from the MSR.
+ */
+static inline void msr_set(unsigned int reg, uint64_t set)
+{
+	msr_unset_and_set(reg, 0, set);
+}
+
+/**
+ * Helper for unsetting MSR bitmasks
+ *
+ * @param[in] reg	The MSR.
+ * @param[in] unset	Bitmask with ones to the bits to unset from the MSR.
+ */
+static inline void msr_unset(unsigned int reg, uint64_t unset)
+{
+	msr_unset_and_set(reg, unset, 0);
+}
+
 #endif /* __ASSEMBLER__ */
 #endif /* CPU_X86_MSR_H */
diff --git a/src/soc/intel/common/block/cpu/cpulib.c b/src/soc/intel/common/block/cpu/cpulib.c
index cbf9b1b..9092df1 100644
--- a/src/soc/intel/common/block/cpu/cpulib.c
+++ b/src/soc/intel/common/block/cpu/cpulib.c
@@ -337,7 +337,7 @@

void cpu_lt_lock_memory(void *unused)
 {
-	msr_set_bit(MSR_LT_CONTROL, LT_CONTROL_LOCK_BIT);
+	msr_set(MSR_LT_CONTROL, LT_CONTROL_LOCK);
 }

int get_valid_prmrr_size(void)
diff --git a/src/soc/intel/common/block/include/intelblocks/msr.h b/src/soc/intel/common/block/include/intelblocks/msr.h
index 4aa069e..2ef4561 100644
--- a/src/soc/intel/common/block/include/intelblocks/msr.h
+++ b/src/soc/intel/common/block/include/intelblocks/msr.h
@@ -56,7 +56,7 @@
 #define  POWER_CTL_C1E_MASK		(1 << 1)
 #define MSR_EVICT_CTL			0x2e0
 #define MSR_LT_CONTROL			0x2e7
-#define  LT_CONTROL_LOCK_BIT		(0)
+#define  LT_CONTROL_LOCK		(1 << 0)
 #define MSR_SGX_OWNEREPOCH0		0x300
 #define MSR_SGX_OWNEREPOCH1		0x301
 #define SMM_FEATURE_CONTROL_MSR		0x4e0