[PATCH v2 2/2] x86/cpu/intel: Clean-up TME code

[PATCH v2 2/2] x86/cpu/intel: Clean-up TME code

[Compostella, Jeremy]

This patch improves the intel TME code quality by:
1. Moving the TME MSR definition to msr-index.h
2. Making the `mktme_status' variable an enum local to the
   detect_tme() function as this is the only function using it.

Signed-off-by: Jeremy Compostella <jeremy.compostella@intel.com>
---
 arch/x86/include/asm/msr-index.h | 11 +++++++++++
 arch/x86/kernel/cpu/intel.c      | 32 ++++++++++++++++++--------------
 2 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 1d111350197f..25303194cf5a 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -1092,6 +1092,17 @@
 #define VMX_BASIC_MEM_TYPE_WB	6LLU
 #define VMX_BASIC_INOUT		0x0040000000000000LLU
 
+/* Total Memory Encryption */
+#define MSR_IA32_TME_ACTIVATE			0x982
+#define MSR_IA32_TME_ACTIVATE_LOCKED		BIT(0)
+#define MSR_IA32_TME_ACTIVATE_ENABLED		BIT(1)
+#define MSR_IA32_TME_ACTIVATE_POLICY_OFFSET	4
+#define MSR_IA32_TME_ACTIVATE_POLICY_MASK	0xf
+#define MSR_IA32_TME_ACTIVATE_KEYID_BITS_OFFSET	32
+#define MSR_IA32_TME_ACTIVATE_KEYID_BITS_MASK	0xf
+#define MSR_IA32_TME_ACTIVATE_CRYPTO_ALG_OFFSET	48
+#define MSR_IA32_TME_ACTIVATE_CRYPTO_ALG_MASK	0xffff
+
 /* Resctrl MSRs: */
 /* - Intel: */
 #define MSR_IA32_L3_QOS_CFG		0xc81
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 17240b96ffda..88c95d919061 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -184,31 +184,34 @@ static bool bad_spectre_microcode(struct cpuinfo_x86 *c)
 	return false;
 }
 
-#define MSR_IA32_TME_ACTIVATE		0x982
-
 /* Helpers to access TME_ACTIVATE MSR */
-#define TME_ACTIVATE_LOCKED(x)		(x & 0x1)
-#define TME_ACTIVATE_ENABLED(x)		(x & 0x2)
+#define TME_ACTIVATE_IS_LOCKED(x)	(x & MSR_IA32_TME_ACTIVATE_LOCKED)
+#define TME_ACTIVATE_IS_ENABLED(x)	(x & MSR_IA32_TME_ACTIVATE_ENABLED)
 
-#define TME_ACTIVATE_POLICY(x)		((x >> 4) & 0xf)	/* Bits 7:4 */
+#define TME_ACTIVATE_POLICY(x)	\
+	((x >> MSR_IA32_TME_ACTIVATE_POLICY_OFFSET)	\
+	 & MSR_IA32_TME_ACTIVATE_POLICY_MASK)
 #define TME_ACTIVATE_POLICY_AES_XTS_128	0
 
-#define TME_ACTIVATE_KEYID_BITS(x)	((x >> 32) & 0xf)	/* Bits 35:32 */
+#define TME_ACTIVATE_KEYID_BITS(x)			\
+	((x >> MSR_IA32_TME_ACTIVATE_KEYID_BITS_OFFSET)	\
+	 & MSR_IA32_TME_ACTIVATE_KEYID_BITS_MASK)
 
-#define TME_ACTIVATE_CRYPTO_ALGS(x)	((x >> 48) & 0xffff)	/* Bits 63:48 */
+#define TME_ACTIVATE_CRYPTO_ALGS(x)			\
+	((x >> MSR_IA32_TME_ACTIVATE_CRYPTO_ALG_OFFSET)	\
+	 & MSR_IA32_TME_ACTIVATE_CRYPTO_ALG_MASK)
 #define TME_ACTIVATE_CRYPTO_AES_XTS_128	1
 
-/* Values for mktme_status (SW only construct) */
-#define MKTME_ENABLED			0
-#define MKTME_DISABLED			1
-#define MKTME_UNINITIALIZED		2
-static int mktme_status = MKTME_UNINITIALIZED;
-
 static void detect_tme(struct cpuinfo_x86 *c)
 {
 	u64 tme_activate, tme_policy, tme_crypto_algs;
 	int keyid_bits = 0, nr_keyids = 0;
 	static u64 tme_activate_cpu0 = 0;
+	static enum {
+		MKTME_ENABLED,
+		MKTME_DISABLED,
+		MKTME_UNINITIALIZED
+	} mktme_status = MKTME_UNINITIALIZED;
 
 	rdmsrl(MSR_IA32_TME_ACTIVATE, tme_activate);
 
@@ -225,7 +228,8 @@ static void detect_tme(struct cpuinfo_x86 *c)
 		tme_activate_cpu0 = tme_activate;
 	}
 
-	if (!TME_ACTIVATE_LOCKED(tme_activate) || !TME_ACTIVATE_ENABLED(tme_activate)) {
+	if (!TME_ACTIVATE_IS_LOCKED(tme_activate) ||
+	    !TME_ACTIVATE_IS_ENABLED(tme_activate)) {
 		pr_info_once("x86/tme: not enabled by BIOS\n");
 		mktme_status = MKTME_DISABLED;
 		return;
-- 
2.40.1

Re: [PATCH v2 2/2] x86/cpu/intel: Clean-up TME code

[Ingo Molnar]

* Compostella, Jeremy <jeremy.compostella@intel.com> wrote:

> -/* Values for mktme_status (SW only construct) */
> -#define MKTME_ENABLED			0
> -#define MKTME_DISABLED			1
> -#define MKTME_UNINITIALIZED		2
> -static int mktme_status = MKTME_UNINITIALIZED;
> -
>  static void detect_tme(struct cpuinfo_x86 *c)
>  {
>  	u64 tme_activate, tme_policy, tme_crypto_algs;
>  	int keyid_bits = 0, nr_keyids = 0;
>  	static u64 tme_activate_cpu0 = 0;
> +	static enum {
> +		MKTME_ENABLED,
> +		MKTME_DISABLED,
> +		MKTME_UNINITIALIZED
> +	} mktme_status = MKTME_UNINITIALIZED;

Please don't move the new enum inside the function, even if
that's the only place it's used.

We almost always keep constant definitions outside function scope.

We also try to avoid static variables inside functions.

Thanks,

	Ingo