[PATCHv2 0/5] x86: Enumerate TME and PCONFIG, add MKTME_KEY_PROG helper

[PATCHv2 0/5] x86: Enumerate TME and PCONFIG, add MKTME_KEY_PROG helper

[Kirill A. Shutemov]

The patchset does some ground work for MKTME enabling:
  - Adds two new cpufeatures: TME and PCONFIG;
  - Detects if BIOS enabled TME and MKTME;
  - Enumerates what PCONFIG targets are supported;
  - Provides helper to program encryption keys into CPU;

As part of TME enumeration we check of how many bits from physical address
are claimed for encryption key ID. This may be critical as we or guest VM
must not use these bits for physical address.

Please review and consider applying.

v2:
  - Fixes for TME enumeration;
  - Add PCONFIG CPUID leaf support;
  - Add MKTME_KEY_PROG helper;

[1] https://software.intel.com/sites/default/files/managed/a5/16/Multi-Key-Total-Memory-Encryption-Spec.pdf

Kirill A. Shutemov (5):
  x86/cpufeatures: Add Intel Total Memory Encryption cpufeature
  x86/tme: Detect if TME and MKTME is activated by BIOS
  x86/cpufeatures: Add Intel PCONFIG cpufeature
  x86/pconfig: Detect PCONFIG targets
  x86/pconfig: Provide defines and helper to run MKTME_KEY_PROG leaf

 arch/x86/include/asm/cpufeatures.h   |  2 +
 arch/x86/include/asm/intel_pconfig.h | 65 +++++++++++++++++++++++++
 arch/x86/kernel/cpu/Makefile         |  2 +-
 arch/x86/kernel/cpu/intel.c          | 94 ++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/cpu/intel_pconfig.c  | 82 +++++++++++++++++++++++++++++++
 5 files changed, 244 insertions(+), 1 deletion(-)
 create mode 100644 arch/x86/include/asm/intel_pconfig.h
 create mode 100644 arch/x86/kernel/cpu/intel_pconfig.c

-- 
2.15.1

[PATCHv2 1/5] x86/cpufeatures: Add Intel Total Memory Encryption cpufeature

[Kirill A. Shutemov]

CPUID.0x7.0x0:ECX[13] indicates whether CPU supports Intel Total Memory
Encryption.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 arch/x86/include/asm/cpufeatures.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 14c3aa2b5f90..d3702d9ac012 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -315,6 +315,7 @@
 #define X86_FEATURE_VPCLMULQDQ		(16*32+10) /* Carry-Less Multiplication Double Quadword */
 #define X86_FEATURE_AVX512_VNNI		(16*32+11) /* Vector Neural Network Instructions */
 #define X86_FEATURE_AVX512_BITALG	(16*32+12) /* Support for VPOPCNT[B,W] and VPSHUF-BITQMB instructions */
+#define X86_FEATURE_TME			(16*32+13) /* Intel Total Memory Encryption */
 #define X86_FEATURE_AVX512_VPOPCNTDQ	(16*32+14) /* POPCNT for vectors of DW/QW */
 #define X86_FEATURE_LA57		(16*32+16) /* 5-level page tables */
 #define X86_FEATURE_RDPID		(16*32+22) /* RDPID instruction */
-- 
2.15.1

[PATCHv2 2/5] x86/tme: Detect if TME and MKTME is activated by BIOS

[Kirill A. Shutemov]

IA32_TME_ACTIVATE MSR (0x982) can be used to check if BIOS has enabled
TME and MKTME. It includes which encryption policy/algorithm is selected
for TME or available for MKTME. For MKTME, the MSR also enumerates how
many KeyIDs are available.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 arch/x86/kernel/cpu/intel.c | 94 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 94 insertions(+)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 319bf989fad1..5f8e37675329 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -506,6 +506,97 @@ static void detect_vmx_virtcap(struct cpuinfo_x86 *c)
 	}
 }
 
+#define MSR_IA32_TME_ACTIVATE		0x982
+
+#define TME_ACTIVATE_LOCKED(x)		(x & 0x1)
+#define TME_ACTIVATE_ENABLED(x)		(x & 0x2)
+
+#define TME_ACTIVATE_POLICY(x)		((x >> 4) & 0xf)	/* Bits 7:4 */
+#define TME_ACTIVATE_POLICY_AES_XTS_128	0
+
+#define TME_ACTIVATE_KEYID_BITS(x)	((x >> 32) & 0xf)	/* Bits 35:32 */
+
+#define TME_ACTIVATE_CRYPTO_ALGS(x)	((x >> 48) & 0xffff)	/* Bits 63:48 */
+#define TME_ACTIVATE_CRYPTO_AES_XTS_128	1
+
+#define MKTME_ENABLED		0
+#define MKTME_DISABLED		1
+#define MKTME_UNINITIALIZED	2
+static int mktme_status = MKTME_UNINITIALIZED;
+
+static void detect_keyid_bits(struct cpuinfo_x86 *c, u64 tme_activate)
+{
+	int keyid_bits = 0, nr_keyids = 0;
+
+	keyid_bits = TME_ACTIVATE_KEYID_BITS(tme_activate);
+	nr_keyids = (1UL << keyid_bits) - 1;
+	if (nr_keyids) {
+		pr_info_once("x86/mktme: enabled by BIOS\n");
+		pr_info_once("x86/mktme: %d KeyIDs available\n", nr_keyids);
+	} else {
+		pr_info_once("x86/mktme: disabled by BIOS\n");
+	}
+
+	if (mktme_status == MKTME_UNINITIALIZED) {
+		/* MKTME is usable */
+		mktme_status = MKTME_ENABLED;
+	}
+
+	/*
+	 * Exclude KeyID bits from physical address bits.
+	 *
+	 * We have to do this even if we are not going to use KeyID bits
+	 * ourself. VM guests still have to know that these bits are not usable
+	 * for physical address.
+	 */
+	c->x86_phys_bits -= keyid_bits;
+}
+
+static void detect_tme(struct cpuinfo_x86 *c)
+{
+	u64 tme_activate, tme_policy, tme_crypto_algs;
+	static u64 tme_activate_cpu0 = 0;
+
+	rdmsrl(MSR_IA32_TME_ACTIVATE, tme_activate);
+
+	if (mktme_status != MKTME_UNINITIALIZED) {
+		if (tme_activate != tme_activate_cpu0) {
+			/* Broken BIOS? */
+			pr_err_once("x86/tme: configuation is inconsistent between CPUs\n");
+			pr_err_once("x86/tme: MKTME is not usable\n");
+			mktme_status = MKTME_DISABLED;
+
+			/* Proceed. We may need to exclude bits from x86_phys_bits. */
+		}
+	} else {
+		tme_activate_cpu0 = tme_activate;
+	}
+
+	if (!TME_ACTIVATE_LOCKED(tme_activate) || !TME_ACTIVATE_ENABLED(tme_activate)) {
+		pr_info_once("x86/tme: not enabled by BIOS\n");
+		mktme_status = MKTME_DISABLED;
+		return;
+	}
+
+	if (mktme_status != MKTME_UNINITIALIZED)
+		return detect_keyid_bits(c, tme_activate);
+
+	pr_info("x86/tme: enabled by BIOS\n");
+
+	tme_policy = TME_ACTIVATE_POLICY(tme_activate);
+	if (tme_policy != TME_ACTIVATE_POLICY_AES_XTS_128)
+		pr_warn("x86/tme: Unknown policy is active: %#llx\n", tme_policy);
+
+	tme_crypto_algs = TME_ACTIVATE_CRYPTO_ALGS(tme_activate);
+	if (!(tme_crypto_algs & TME_ACTIVATE_CRYPTO_AES_XTS_128)) {
+		pr_err("x86/mktme: No known encryption algorithm is supported: %#llx\n",
+				tme_crypto_algs);
+		mktme_status = MKTME_DISABLED;
+	}
+
+	detect_keyid_bits(c, tme_activate);
+}
+
 static void init_intel_energy_perf(struct cpuinfo_x86 *c)
 {
 	u64 epb;
@@ -676,6 +767,9 @@ static void init_intel(struct cpuinfo_x86 *c)
 	if (cpu_has(c, X86_FEATURE_VMX))
 		detect_vmx_virtcap(c);
 
+	if (cpu_has(c, X86_FEATURE_TME))
+		detect_tme(c);
+
 	init_intel_energy_perf(c);
 
 	init_intel_misc_features(c);
-- 
2.15.1

[PATCHv2 3/5] x86/cpufeatures: Add Intel PCONFIG cpufeature

[Kirill A. Shutemov]

CPUID.0x7.0x0:EDX[18] indicates whether Intel CPU support PCONFIG instruction.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 arch/x86/include/asm/cpufeatures.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index d3702d9ac012..b9b46b593938 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -328,6 +328,7 @@
 /* Intel-defined CPU features, CPUID level 0x00000007:0 (EDX), word 18 */
 #define X86_FEATURE_AVX512_4VNNIW	(18*32+ 2) /* AVX-512 Neural Network Instructions */
 #define X86_FEATURE_AVX512_4FMAPS	(18*32+ 3) /* AVX-512 Multiply Accumulation Single precision */
+#define X86_FEATURE_PCONFIG		(18*32+18) /* Intel PCONFIG */
 #define X86_FEATURE_SPEC_CTRL		(18*32+26) /* "" Speculation Control (IBRS + IBPB) */
 #define X86_FEATURE_INTEL_STIBP		(18*32+27) /* "" Single Thread Indirect Branch Predictors */
 #define X86_FEATURE_ARCH_CAPABILITIES	(18*32+29) /* IA32_ARCH_CAPABILITIES MSR (Intel) */
-- 
2.15.1

[PATCHv2 4/5] x86/pconfig: Detect PCONFIG targets

[Kirill A. Shutemov]

Intel PCONFIG targets are enumerated via new CPUID leaf 0x1b. This patch
detects all supported targets of PCONFIG and implements helper to check
if the target is supported.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 arch/x86/include/asm/intel_pconfig.h | 15 +++++++
 arch/x86/kernel/cpu/Makefile         |  2 +-
 arch/x86/kernel/cpu/intel_pconfig.c  | 82 ++++++++++++++++++++++++++++++++++++
 3 files changed, 98 insertions(+), 1 deletion(-)
 create mode 100644 arch/x86/include/asm/intel_pconfig.h
 create mode 100644 arch/x86/kernel/cpu/intel_pconfig.c

diff --git a/arch/x86/include/asm/intel_pconfig.h b/arch/x86/include/asm/intel_pconfig.h
new file mode 100644
index 000000000000..fb7a37c3798b
--- /dev/null
+++ b/arch/x86/include/asm/intel_pconfig.h
@@ -0,0 +1,15 @@
+#ifndef	_ASM_X86_INTEL_PCONFIG_H
+#define	_ASM_X86_INTEL_PCONFIG_H
+
+#include <asm/asm.h>
+#include <asm/processor.h>
+
+enum pconfig_target {
+	INVALID_TARGET	= 0,
+	MKTME_TARGET	= 1,
+	PCONFIG_TARGET_NR
+};
+
+int pconfig_target_supported(enum pconfig_target target);
+
+#endif	/* _ASM_X86_INTEL_PCONFIG_H */
diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
index 570e8bb1f386..a66229f51b12 100644
--- a/arch/x86/kernel/cpu/Makefile
+++ b/arch/x86/kernel/cpu/Makefile
@@ -28,7 +28,7 @@ obj-y			+= cpuid-deps.o
 obj-$(CONFIG_PROC_FS)	+= proc.o
 obj-$(CONFIG_X86_FEATURE_NAMES) += capflags.o powerflags.o
 
-obj-$(CONFIG_CPU_SUP_INTEL)		+= intel.o
+obj-$(CONFIG_CPU_SUP_INTEL)		+= intel.o intel_pconfig.o
 obj-$(CONFIG_CPU_SUP_AMD)		+= amd.o
 obj-$(CONFIG_CPU_SUP_CYRIX_32)		+= cyrix.o
 obj-$(CONFIG_CPU_SUP_CENTAUR)		+= centaur.o
diff --git a/arch/x86/kernel/cpu/intel_pconfig.c b/arch/x86/kernel/cpu/intel_pconfig.c
new file mode 100644
index 000000000000..0771a905b286
--- /dev/null
+++ b/arch/x86/kernel/cpu/intel_pconfig.c
@@ -0,0 +1,82 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel PCONFIG instruction support.
+ *
+ * Copyright (C) 2017 Intel Corporation
+ *
+ * Author:
+ *	Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
+ */
+
+#include <asm/cpufeature.h>
+#include <asm/intel_pconfig.h>
+
+#define	PCONFIG_CPUID			0x1b
+
+#define PCONFIG_CPUID_SUBLEAF_MASK	((1 << 12) - 1)
+
+/* Subleaf type (EAX) for PCONFIG CPUID leaf (0x1B) */
+enum {
+	PCONFIG_CPUID_SUBLEAF_INVALID	= 0,
+	PCONFIG_CPUID_SUBLEAF_TARGETID	= 1,
+};
+
+/* Bitmask of supported targets */
+static u64 targets_supported __read_mostly;
+
+int pconfig_target_supported(enum pconfig_target target)
+{
+	/*
+	 * We would need to re-think the implementation once we get > 64
+	 * PCONFIG targets. Spec allows up to 2^32 targets.
+	 */
+	BUILD_BUG_ON(PCONFIG_TARGET_NR >= 64);
+
+	if (WARN_ON_ONCE(target >= 64))
+		return 0;
+	return targets_supported & (1ULL << target);
+}
+
+static int __init intel_pconfig_init(void)
+{
+	int subleaf;
+
+	if (!boot_cpu_has(X86_FEATURE_PCONFIG))
+		return 0;
+
+	/*
+	 * Scan subleafs of PCONFIG CPUID leaf.
+	 *
+	 * Subleafs of the same type need not to be consecutive.
+	 *
+	 * Stop on the first invalid subleaf type. All subleafs after the first
+	 * invalid are invalid too.
+	 */
+	for (subleaf = 0; subleaf < INT_MAX; subleaf++) {
+		struct cpuid_regs regs;
+
+		cpuid_count(PCONFIG_CPUID, subleaf,
+				&regs.eax, &regs.ebx, &regs.ecx, &regs.edx);
+
+		switch (regs.eax & PCONFIG_CPUID_SUBLEAF_MASK) {
+		case PCONFIG_CPUID_SUBLEAF_INVALID:
+			/* Stop on the first invalid subleaf */
+			goto out;
+		case PCONFIG_CPUID_SUBLEAF_TARGETID:
+			/* Mark supported PCONFIG targets */
+			if (regs.ebx < 64)
+				targets_supported |= (1ULL << regs.ebx);
+			if (regs.ecx < 64)
+				targets_supported |= (1ULL << regs.ecx);
+			if (regs.edx < 64)
+				targets_supported |= (1ULL << regs.edx);
+			break;
+		default:
+			/* Unknown CPUID.PCONFIG subleaf: ignore */
+			break;
+		}
+	}
+out:
+	return 0;
+}
+arch_initcall(intel_pconfig_init);
-- 
2.15.1

[PATCHv2 5/5] x86/pconfig: Provide defines and helper to run MKTME_KEY_PROG leaf

[Kirill A. Shutemov]

MKTME_KEY_PROG allows to manipulate MKTME keys in the CPU.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 arch/x86/include/asm/intel_pconfig.h | 50 ++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/arch/x86/include/asm/intel_pconfig.h b/arch/x86/include/asm/intel_pconfig.h
index fb7a37c3798b..3cb002b1d0f9 100644
--- a/arch/x86/include/asm/intel_pconfig.h
+++ b/arch/x86/include/asm/intel_pconfig.h
@@ -12,4 +12,54 @@ enum pconfig_target {
 
 int pconfig_target_supported(enum pconfig_target target);
 
+enum pconfig_leaf {
+	MKTME_KEY_PROGRAM	= 0,
+	PCONFIG_LEAF_INVALID,
+};
+
+#define PCONFIG ".byte 0x0f, 0x01, 0xc5"
+
+/* Defines and structure for MKTME_KEY_PROGRAM of PCONFIG instruction */
+
+/* mktme_key_program::keyid_ctrl COMMAND, bits [7:0] */
+#define MKTME_KEYID_SET_KEY_DIRECT	0
+#define MKTME_KEYID_SET_KEY_RANDOM	1
+#define MKTME_KEYID_CLEAR_KEY		2
+#define MKTME_KEYID_NO_ENCRYPT		3
+
+/* mktme_key_program::keyid_ctrl ENC_ALG, bits [23:8] */
+#define MKTME_AES_XTS_128	(1 << 8)
+
+/* Return codes from the PCONFIG MKTME_KEY_PROGRAM */
+#define MKTME_PROG_SUCCESS	0
+#define MKTME_INVALID_PROG_CMD	1
+#define MKTME_ENTROPY_ERROR	2
+#define MKTME_INVALID_KEYID	3
+#define MKTME_INVALID_ENC_ALG	4
+#define MKTME_DEVICE_BUSY	5
+
+/* Hardware requires the structure to be 256 byte alinged. Otherwise #GP(0). */
+struct mktme_key_program {
+	u16 keyid;
+	u32 keyid_ctrl;
+	u8 __rsvd[58];
+	u8 key_field_1[64];
+	u8 key_field_2[64];
+} __packed __aligned(256);
+
+static inline int mktme_key_program(struct mktme_key_program *key_program)
+{
+	unsigned long rax = MKTME_KEY_PROGRAM;
+
+	if (!pconfig_target_supported(MKTME_TARGET))
+		return -ENXIO;
+
+	asm volatile(PCONFIG
+		: "=a" (rax), "=b" (key_program)
+		: "0" (rax), "1" (key_program)
+		: "memory", "cc");
+
+	return rax;
+}
+
 #endif	/* _ASM_X86_INTEL_PCONFIG_H */
-- 
2.15.1

Re: [PATCHv2 0/5] x86: Enumerate TME and PCONFIG, add MKTME_KEY_PROG helper

[Dave Hansen]

On 02/07/2018 04:59 AM, Kirill A. Shutemov wrote:
> The patchset does some ground work for MKTME enabling:
>   - Adds two new cpufeatures: TME and PCONFIG;
>   - Detects if BIOS enabled TME and MKTME;
>   - Enumerates what PCONFIG targets are supported;
>   - Provides helper to program encryption keys into CPU;
> 
> As part of TME enumeration we check of how many bits from physical address
> are claimed for encryption key ID. This may be critical as we or guest VM
> must not use these bits for physical address.

For this kind of stuff, I'd really appreciate if you included some
high-level descriptions.  I'd assume that a reviewer has no idea what
PCONFIG or MKTME is.

It would also be really nice to say which hardware will implement this.
Is it in particular CPUs today, for instance?

Re: [PATCHv2 2/5] x86/tme: Detect if TME and MKTME is activated by BIOS

[Dave Hansen]

On 02/07/2018 04:59 AM, Kirill A. Shutemov wrote:
> IA32_TME_ACTIVATE MSR (0x982) can be used to check if BIOS has enabled
> TME and MKTME. It includes which encryption policy/algorithm is selected
> for TME or available for MKTME. For MKTME, the MSR also enumerates how
> many KeyIDs are available.

The hacking of the phys_addr_bits is a pretty important part of this.
Are you sure it's not worth calling out in the description?

> +#define MSR_IA32_TME_ACTIVATE		0x982
> +
> +#define TME_ACTIVATE_LOCKED(x)		(x & 0x1)
> +#define TME_ACTIVATE_ENABLED(x)		(x & 0x2)
> +
> +#define TME_ACTIVATE_POLICY(x)		((x >> 4) & 0xf)	/* Bits 7:4 */
> +#define TME_ACTIVATE_POLICY_AES_XTS_128	0
> +
> +#define TME_ACTIVATE_KEYID_BITS(x)	((x >> 32) & 0xf)	/* Bits 35:32 */
> +
> +#define TME_ACTIVATE_CRYPTO_ALGS(x)	((x >> 48) & 0xffff)	/* Bits 63:48 */
> +#define TME_ACTIVATE_CRYPTO_AES_XTS_128	1
> +
> +#define MKTME_ENABLED		0
> +#define MKTME_DISABLED		1
> +#define MKTME_UNINITIALIZED	2

The indentation there looks a bit wonky.  Might want to double-check it.

Also, can you clearly spell out which of these things are software
constructs vs. hardware ones?  MKTME_* look like software constructs.

> +static int mktme_status = MKTME_UNINITIALIZED;
> +
> +static void detect_keyid_bits(struct cpuinfo_x86 *c, u64 tme_activate)
> +{
> +	int keyid_bits = 0, nr_keyids = 0;
> +
> +	keyid_bits = TME_ACTIVATE_KEYID_BITS(tme_activate);
> +	nr_keyids = (1UL << keyid_bits) - 1;
> +	if (nr_keyids) {
> +		pr_info_once("x86/mktme: enabled by BIOS\n");
> +		pr_info_once("x86/mktme: %d KeyIDs available\n", nr_keyids);
> +	} else {
> +		pr_info_once("x86/mktme: disabled by BIOS\n");
> +	}

Just curious, but how do you know that this indicates the BIOS disabling
MKTME?

> +	if (mktme_status == MKTME_UNINITIALIZED) {
> +		/* MKTME is usable */
> +		mktme_status = MKTME_ENABLED;
> +	}

To me, it's a little bit odd that we "enable" MKTME down in the keyid
detection code.  I wonder if you could just return the resulting number
of keyids and then actually do the mktme_status munging in one place
(detect_tme()).

> +	/*
> +	 * Exclude KeyID bits from physical address bits.
> +	 *
> +	 * We have to do this even if we are not going to use KeyID bits
> +	 * ourself. VM guests still have to know that these bits are not usable
> +	 * for physical address.
> +	 */
> +	c->x86_phys_bits -= keyid_bits;
> +}

How do we tell guests about this?  kvm_set_mmio_spte_mask()?

> +static void detect_tme(struct cpuinfo_x86 *c)
> +{
> +	u64 tme_activate, tme_policy, tme_crypto_algs;
> +	static u64 tme_activate_cpu0 = 0;
> +
> +	rdmsrl(MSR_IA32_TME_ACTIVATE, tme_activate);
> +
> +	if (mktme_status != MKTME_UNINITIALIZED) {
> +		if (tme_activate != tme_activate_cpu0) {
> +			/* Broken BIOS? */
> +			pr_err_once("x86/tme: configuation is inconsistent between CPUs\n");
> +			pr_err_once("x86/tme: MKTME is not usable\n");
> +			mktme_status = MKTME_DISABLED;
> +
> +			/* Proceed. We may need to exclude bits from x86_phys_bits. */
> +		}
> +	} else {
> +		tme_activate_cpu0 = tme_activate;
> +	}
> +
> +	if (!TME_ACTIVATE_LOCKED(tme_activate) || !TME_ACTIVATE_ENABLED(tme_activate)) {
> +		pr_info_once("x86/tme: not enabled by BIOS\n");
> +		mktme_status = MKTME_DISABLED;
> +		return;
> +	}
> +
> +	if (mktme_status != MKTME_UNINITIALIZED)
> +		return detect_keyid_bits(c, tme_activate);

Returning the result of a void function is a bit odd-looking.  Would it
look nicer to just have a label and some gotos to the detection?

> +	pr_info("x86/tme: enabled by BIOS\n");
> +
> +	tme_policy = TME_ACTIVATE_POLICY(tme_activate);
> +	if (tme_policy != TME_ACTIVATE_POLICY_AES_XTS_128)
> +		pr_warn("x86/tme: Unknown policy is active: %#llx\n", tme_policy);
> +
> +	tme_crypto_algs = TME_ACTIVATE_CRYPTO_ALGS(tme_activate);
> +	if (!(tme_crypto_algs & TME_ACTIVATE_CRYPTO_AES_XTS_128)) {
> +		pr_err("x86/mktme: No known encryption algorithm is supported: %#llx\n",
> +				tme_crypto_algs);
> +		mktme_status = MKTME_DISABLED;
> +	}
> +
> +	detect_keyid_bits(c, tme_activate);
> +}

I noticed that this code is not optional, other than by disabling
CPU_SUP_INTEL.  Was that intentional?  What were your thoughts behind that?

Re: [PATCHv2 2/5] x86/tme: Detect if TME and MKTME is activated by BIOS

[Kai Huang]

On Wed, 2018-02-07 at 11:02 -0800, Dave Hansen wrote:
> On 02/07/2018 04:59 AM, Kirill A. Shutemov wrote:
> > IA32_TME_ACTIVATE MSR (0x982) can be used to check if BIOS has
> > enabled
> > TME and MKTME. It includes which encryption policy/algorithm is
> > selected
> > for TME or available for MKTME. For MKTME, the MSR also enumerates
> > how
> > many KeyIDs are available.
> 
> The hacking of the phys_addr_bits is a pretty important part of this.
> Are you sure it's not worth calling out in the description?
> 
> > +#define MSR_IA32_TME_ACTIVATE		0x982
> > +
> > +#define TME_ACTIVATE_LOCKED(x)		(x & 0x1)
> > +#define TME_ACTIVATE_ENABLED(x)		(x & 0x2)
> > +
> > +#define TME_ACTIVATE_POLICY(x)		((x >> 4) & 0xf)	
> > /* Bits 7:4 */
> > +#define TME_ACTIVATE_POLICY_AES_XTS_128	0
> > +
> > +#define TME_ACTIVATE_KEYID_BITS(x)	((x >> 32) & 0xf)	
> > /* Bits 35:32 */
> > +
> > +#define TME_ACTIVATE_CRYPTO_ALGS(x)	((x >> 48) & 0xffff)	
> > /* Bits 63:48 */
> > +#define TME_ACTIVATE_CRYPTO_AES_XTS_128	1
> > +
> > +#define MKTME_ENABLED		0
> > +#define MKTME_DISABLED		1
> > +#define MKTME_UNINITIALIZED	2
> 
> The indentation there looks a bit wonky.  Might want to double-check
> it.
> 
> Also, can you clearly spell out which of these things are software
> constructs vs. hardware ones?  MKTME_* look like software constructs.
> 
> > +static int mktme_status = MKTME_UNINITIALIZED;
> > +
> > +static void detect_keyid_bits(struct cpuinfo_x86 *c, u64
> > tme_activate)
> > +{
> > +	int keyid_bits = 0, nr_keyids = 0;
> > +
> > +	keyid_bits = TME_ACTIVATE_KEYID_BITS(tme_activate);
> > +	nr_keyids = (1UL << keyid_bits) - 1;
> > +	if (nr_keyids) {
> > +		pr_info_once("x86/mktme: enabled by BIOS\n");
> > +		pr_info_once("x86/mktme: %d KeyIDs available\n",
> > nr_keyids);
> > +	} else {
> > +		pr_info_once("x86/mktme: disabled by BIOS\n");
> > +	}
> 
> Just curious, but how do you know that this indicates the BIOS
> disabling
> MKTME?
> 
> > +	if (mktme_status == MKTME_UNINITIALIZED) {
> > +		/* MKTME is usable */
> > +		mktme_status = MKTME_ENABLED;
> > +	}
> 
> To me, it's a little bit odd that we "enable" MKTME down in the keyid
> detection code.  I wonder if you could just return the resulting
> number
> of keyids and then actually do the mktme_status munging in one place
> (detect_tme()).
> 
> > +	/*
> > +	 * Exclude KeyID bits from physical address bits.
> > +	 *
> > +	 * We have to do this even if we are not going to use
> > KeyID bits
> > +	 * ourself. VM guests still have to know that these bits
> > are not usable
> > +	 * for physical address.
> > +	 */
> > +	c->x86_phys_bits -= keyid_bits;
> > +}
> 
> How do we tell guests about this?  kvm_set_mmio_spte_mask()?

Hi Dave,

KVM tells guest physical bits info in CPUID emulating from guest.
Currently KVM uses native CPUID to get physical bits info, and report
it to guest in CPUID emulating. KVM is not consulting c->x86_phys_bits
in CPUID emulation now, but for MK-TME I think it should be reasonable
for KVM to do that.

The kvm_set_mmio_spte_mask() you mentioned is used to setup pte mask to
cause page fault for guest's MMIO pages (in shadow MMU mode only, for
EPT we have different function). It is using
boot_cpu_data.x86_phys_bits (for which we need to do code update for
MK-TME), but this function is not related to reporting physical bits
info to guest.

Thanks,
-Kai
> 
> > +static void detect_tme(struct cpuinfo_x86 *c)
> > +{
> > +	u64 tme_activate, tme_policy, tme_crypto_algs;
> > +	static u64 tme_activate_cpu0 = 0;
> > +
> > +	rdmsrl(MSR_IA32_TME_ACTIVATE, tme_activate);
> > +
> > +	if (mktme_status != MKTME_UNINITIALIZED) {
> > +		if (tme_activate != tme_activate_cpu0) {
> > +			/* Broken BIOS? */
> > +			pr_err_once("x86/tme: configuation is
> > inconsistent between CPUs\n");
> > +			pr_err_once("x86/tme: MKTME is not
> > usable\n");
> > +			mktme_status = MKTME_DISABLED;
> > +
> > +			/* Proceed. We may need to exclude bits
> > from x86_phys_bits. */
> > +		}
> > +	} else {
> > +		tme_activate_cpu0 = tme_activate;
> > +	}
> > +
> > +	if (!TME_ACTIVATE_LOCKED(tme_activate) ||
> > !TME_ACTIVATE_ENABLED(tme_activate)) {
> > +		pr_info_once("x86/tme: not enabled by BIOS\n");
> > +		mktme_status = MKTME_DISABLED;
> > +		return;
> > +	}
> > +
> > +	if (mktme_status != MKTME_UNINITIALIZED)
> > +		return detect_keyid_bits(c, tme_activate);
> 
> Returning the result of a void function is a bit odd-looking.  Would
> it
> look nicer to just have a label and some gotos to the detection?
> 
> > +	pr_info("x86/tme: enabled by BIOS\n");
> > +
> > +	tme_policy = TME_ACTIVATE_POLICY(tme_activate);
> > +	if (tme_policy != TME_ACTIVATE_POLICY_AES_XTS_128)
> > +		pr_warn("x86/tme: Unknown policy is active:
> > %#llx\n", tme_policy);
> > +
> > +	tme_crypto_algs = TME_ACTIVATE_CRYPTO_ALGS(tme_activate);
> > +	if (!(tme_crypto_algs & TME_ACTIVATE_CRYPTO_AES_XTS_128))
> > {
> > +		pr_err("x86/mktme: No known encryption algorithm
> > is supported: %#llx\n",
> > +				tme_crypto_algs);
> > +		mktme_status = MKTME_DISABLED;
> > +	}
> > +
> > +	detect_keyid_bits(c, tme_activate);
> > +}
> 
> I noticed that this code is not optional, other than by disabling
> CPU_SUP_INTEL.  Was that intentional?  What were your thoughts behind
> that?

Re: [PATCHv2 2/5] x86/tme: Detect if TME and MKTME is activated by BIOS

[Kirill A. Shutemov]

On Wed, Feb 07, 2018 at 11:02:26AM -0800, Dave Hansen wrote:
> On 02/07/2018 04:59 AM, Kirill A. Shutemov wrote:
> > IA32_TME_ACTIVATE MSR (0x982) can be used to check if BIOS has enabled
> > TME and MKTME. It includes which encryption policy/algorithm is selected
> > for TME or available for MKTME. For MKTME, the MSR also enumerates how
> > many KeyIDs are available.
> 
> The hacking of the phys_addr_bits is a pretty important part of this.
> Are you sure it's not worth calling out in the description?

Okay, will do on the next revision.

> > +#define MSR_IA32_TME_ACTIVATE		0x982
> > +
> > +#define TME_ACTIVATE_LOCKED(x)		(x & 0x1)
> > +#define TME_ACTIVATE_ENABLED(x)		(x & 0x2)
> > +
> > +#define TME_ACTIVATE_POLICY(x)		((x >> 4) & 0xf)	/* Bits 7:4 */
> > +#define TME_ACTIVATE_POLICY_AES_XTS_128	0
> > +
> > +#define TME_ACTIVATE_KEYID_BITS(x)	((x >> 32) & 0xf)	/* Bits 35:32 */
> > +
> > +#define TME_ACTIVATE_CRYPTO_ALGS(x)	((x >> 48) & 0xffff)	/* Bits 63:48 */
> > +#define TME_ACTIVATE_CRYPTO_AES_XTS_128	1
> > +
> > +#define MKTME_ENABLED		0
> > +#define MKTME_DISABLED		1
> > +#define MKTME_UNINITIALIZED	2
> 
> The indentation there looks a bit wonky.  Might want to double-check it.

Do you mean that MKTME_* is indented differently than the rest?

I'll fix that.
 
> Also, can you clearly spell out which of these things are software
> constructs vs. hardware ones?  MKTME_* look like software constructs.

Yes, MKTME_* is software. I'll call it out.

> > +static int mktme_status = MKTME_UNINITIALIZED;
> > +
> > +static void detect_keyid_bits(struct cpuinfo_x86 *c, u64 tme_activate)
> > +{
> > +	int keyid_bits = 0, nr_keyids = 0;
> > +
> > +	keyid_bits = TME_ACTIVATE_KEYID_BITS(tme_activate);
> > +	nr_keyids = (1UL << keyid_bits) - 1;
> > +	if (nr_keyids) {
> > +		pr_info_once("x86/mktme: enabled by BIOS\n");
> > +		pr_info_once("x86/mktme: %d KeyIDs available\n", nr_keyids);
> > +	} else {
> > +		pr_info_once("x86/mktme: disabled by BIOS\n");
> > +	}
> 
> Just curious, but how do you know that this indicates the BIOS disabling
> MKTME?

0 bits for KeyID means we don't have MKTME. Only TME.

> 
> > +	if (mktme_status == MKTME_UNINITIALIZED) {
> > +		/* MKTME is usable */
> > +		mktme_status = MKTME_ENABLED;
> > +	}
> 
> To me, it's a little bit odd that we "enable" MKTME down in the keyid
> detection code.  I wonder if you could just return the resulting number
> of keyids and then actually do the mktme_status munging in one place
> (detect_tme()).

Makes sense.

> > +	/*
> > +	 * Exclude KeyID bits from physical address bits.
> > +	 *
> > +	 * We have to do this even if we are not going to use KeyID bits
> > +	 * ourself. VM guests still have to know that these bits are not usable
> > +	 * for physical address.
> > +	 */
> > +	c->x86_phys_bits -= keyid_bits;
> > +}
> 
> How do we tell guests about this?  kvm_set_mmio_spte_mask()?

Has Kai answered this for you?

> > +static void detect_tme(struct cpuinfo_x86 *c)
> > +{
> > +	u64 tme_activate, tme_policy, tme_crypto_algs;
> > +	static u64 tme_activate_cpu0 = 0;
> > +
> > +	rdmsrl(MSR_IA32_TME_ACTIVATE, tme_activate);
> > +
> > +	if (mktme_status != MKTME_UNINITIALIZED) {
> > +		if (tme_activate != tme_activate_cpu0) {
> > +			/* Broken BIOS? */
> > +			pr_err_once("x86/tme: configuation is inconsistent between CPUs\n");
> > +			pr_err_once("x86/tme: MKTME is not usable\n");
> > +			mktme_status = MKTME_DISABLED;
> > +
> > +			/* Proceed. We may need to exclude bits from x86_phys_bits. */
> > +		}
> > +	} else {
> > +		tme_activate_cpu0 = tme_activate;
> > +	}
> > +
> > +	if (!TME_ACTIVATE_LOCKED(tme_activate) || !TME_ACTIVATE_ENABLED(tme_activate)) {
> > +		pr_info_once("x86/tme: not enabled by BIOS\n");
> > +		mktme_status = MKTME_DISABLED;
> > +		return;
> > +	}
> > +
> > +	if (mktme_status != MKTME_UNINITIALIZED)
> > +		return detect_keyid_bits(c, tme_activate);
> 
> Returning the result of a void function is a bit odd-looking.  Would it
> look nicer to just have a label and some gotos to the detection?

Okay. Either way fine to me.

> > +	pr_info("x86/tme: enabled by BIOS\n");
> > +
> > +	tme_policy = TME_ACTIVATE_POLICY(tme_activate);
> > +	if (tme_policy != TME_ACTIVATE_POLICY_AES_XTS_128)
> > +		pr_warn("x86/tme: Unknown policy is active: %#llx\n", tme_policy);
> > +
> > +	tme_crypto_algs = TME_ACTIVATE_CRYPTO_ALGS(tme_activate);
> > +	if (!(tme_crypto_algs & TME_ACTIVATE_CRYPTO_AES_XTS_128)) {
> > +		pr_err("x86/mktme: No known encryption algorithm is supported: %#llx\n",
> > +				tme_crypto_algs);
> > +		mktme_status = MKTME_DISABLED;
> > +	}
> > +
> > +	detect_keyid_bits(c, tme_activate);
> > +}
> 
> I noticed that this code is not optional, other than by disabling
> CPU_SUP_INTEL.  Was that intentional?  What were your thoughts behind that?

We need to mask out bits for KeyID even if we don't use them ourself, so I think
we should do this unconditionally.

I need to re-check this with 32-bit kernel, though.

-- 
 Kirill A. Shutemov