2023-09-26 00:15:09

by Compostella, Jeremy

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

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 <[email protected]>
---
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

2023-09-28 22:46:50

by Ingo Molnar

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

* Compostella, Jeremy <[email protected]> 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