[PATCH 0/3] Enumerate TME and PCONFIG

[PATCH 0/3] Enumerate TME and PCONFIG

[Kirill A. Shutemov]

This patchset enumerates two features required for Multi-Key Total Memory
Encryption enabling[1].

Apart from trivial cpufeatures bits, the patchset enumerates 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.

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

Kirill A. Shutemov (3):
  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

 arch/x86/include/asm/cpufeatures.h |  2 +
 arch/x86/kernel/cpu/intel.c        | 83 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 85 insertions(+)

-- 
2.15.1

[PATCH 1/3] 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

[PATCH 2/3] 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 | 83 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 6936d14d4c77..5b95fa484837 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -517,6 +517,86 @@ 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	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	1
+
+#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;
+
+	rdmsrl(MSR_IA32_TME_ACTIVATE, tme_activate);
+
+	if (mktme_status != MKTME_UNINITIALIZED) {
+		/* Broken BIOS? */
+		if (tme_activate != tme_activate_cpu0) {
+			pr_err_once("TME: configuation is inconsistent between CPUs\n");
+			mktme_status = MKTME_DISABLED;
+		}
+		goto out;
+	}
+
+	tme_activate_cpu0 = tme_activate;
+
+	if (!TME_ACTIVATE_LOCKED(tme_activate) || !TME_ACTIVATE_ENABLED(tme_activate)) {
+		pr_info("TME: not enabled by BIOS\n");
+		mktme_status = MKTME_DISABLED;
+		goto out;
+	}
+
+	pr_info("TME: enabled by BIOS\n");
+
+	tme_policy = TME_ACTIVATE_POLICY(tme_activate);
+	if (tme_policy != TME_ACTIVATE_POLICY_AES_XTS)
+		pr_warn("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)) {
+		pr_err("MKTME: No known encryption algorithm is supported: %#llx\n",
+				tme_crypto_algs);
+		mktme_status = MKTME_DISABLED;
+	}
+out:
+	keyid_bits = TME_ACTIVATE_KEYID_BITS(tme_activate);
+	nr_keyids = (1UL << keyid_bits) - 1;
+	if (nr_keyids) {
+		pr_info_once("MKTME: enabled by BIOS\n");
+		pr_info_once("MKTME: %d KeyIDs available\n", nr_keyids);
+	} else {
+		pr_info_once("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 init_intel_energy_perf(struct cpuinfo_x86 *c)
 {
 	u64 epb;
@@ -687,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

[PATCH 3/3] 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

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

[Kai Huang]

On Wed, 2018-01-31 at 12:15 +0300, 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.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  arch/x86/kernel/cpu/intel.c | 83
> +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 83 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/intel.c
> b/arch/x86/kernel/cpu/intel.c
> index 6936d14d4c77..5b95fa484837 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -517,6 +517,86 @@ static void detect_vmx_virtcap(struct
> cpuinfo_x86 *c)
>  	}
>  }
>  
> +#define MSR_IA32_TME_ACTIVATE		0x982

Should this MSR go into msr-index.h?

> +
> +#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	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	1
> +
> +#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;
> +
> +	rdmsrl(MSR_IA32_TME_ACTIVATE, tme_activate);
> +
> +	if (mktme_status != MKTME_UNINITIALIZED) {
> +		/* Broken BIOS? */
> +		if (tme_activate != tme_activate_cpu0) {
> +			pr_err_once("TME: configuation is
> inconsistent between CPUs\n");
> +			mktme_status = MKTME_DISABLED;
> +		}
> +		goto out;

Why goto out here? If something goes wrong, I think it is pointless to
read keyID bits staff? IMHO if something goes wrong, you should set
mktme_status to disabled, and clear tme_activate_cpu0?

> +	}
> +
> +	tme_activate_cpu0 = tme_activate;
> +
> +	if (!TME_ACTIVATE_LOCKED(tme_activate) ||
> !TME_ACTIVATE_ENABLED(tme_activate)) {
> +		pr_info("TME: not enabled by BIOS\n");
> +		mktme_status = MKTME_DISABLED;
> +		goto out;

I think it is pointless to read keyID bits staff if TME is not even
enabled.

> +	}
> +
> +	pr_info("TME: enabled by BIOS\n");
> +
> +	tme_policy = TME_ACTIVATE_POLICY(tme_activate);
> +	if (tme_policy != TME_ACTIVATE_POLICY_AES_XTS)
> +		pr_warn("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)) {
> +		pr_err("MKTME: No known encryption algorithm is
> supported: %#llx\n",
> +				tme_crypto_algs);
> +		mktme_status = MKTME_DISABLED;
> +	}

To me it is a little bit confusing about the naming. tme_policy is the
crypto_alg used by TME keyID (0), and tme_crypto_algs is bitmap of
supported crypto_algs for MK-TME. Probably a better naming is needed?
And the naming of TME_ACTIVATE_POLICY(x), TME_ACTIVATE_CRYPTO_ALGS(x)
above as well?

> +out:
> +	keyid_bits = TME_ACTIVATE_KEYID_BITS(tme_activate);
> +	nr_keyids = (1UL << keyid_bits) - 1;
> +	if (nr_keyids) {
> +		pr_info_once("MKTME: enabled by BIOS\n");
> +		pr_info_once("MKTME: %d KeyIDs available\n",
> nr_keyids);
> +	} else {
> +		pr_info_once("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.
> +	 */
Currently KVM uses CPUID to get such info directly, but not consulting
c->x86_phys_bits. I think it may be reasonable for KVM to consulting c-
>x86_phys_bits for MK-TME, but IMHO the real reason we need to do this
is this is just the fact, and c->x86_phys_bits needs to reflect the
fact, so probably the comments can be refined.

Thanks,
-Kai

> +	c->x86_phys_bits -= keyid_bits;
> +}
> +
>  static void init_intel_energy_perf(struct cpuinfo_x86 *c)
>  {
>  	u64 epb;
> @@ -687,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);

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

[Kirill A. Shutemov]

On Wed, Feb 07, 2018 at 05:34:10PM +1300, Kai Huang wrote:
> On Wed, 2018-01-31 at 12:15 +0300, 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.
> > 
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > ---
> >  arch/x86/kernel/cpu/intel.c | 83
> > +++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 83 insertions(+)
> > 
> > diff --git a/arch/x86/kernel/cpu/intel.c
> > b/arch/x86/kernel/cpu/intel.c
> > index 6936d14d4c77..5b95fa484837 100644
> > --- a/arch/x86/kernel/cpu/intel.c
> > +++ b/arch/x86/kernel/cpu/intel.c
> > @@ -517,6 +517,86 @@ static void detect_vmx_virtcap(struct
> > cpuinfo_x86 *c)
> >  	}
> >  }
> >  
> > +#define MSR_IA32_TME_ACTIVATE		0x982
> 
> Should this MSR go into msr-index.h?

No. Comment from msr-index.h:

 * Do not add new entries to this file unless the definitions are shared
 * between multiple compilation units.

> > +
> > +#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	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	1
> > +
> > +#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;
> > +
> > +	rdmsrl(MSR_IA32_TME_ACTIVATE, tme_activate);
> > +
> > +	if (mktme_status != MKTME_UNINITIALIZED) {
> > +		/* Broken BIOS? */
> > +		if (tme_activate != tme_activate_cpu0) {
> > +			pr_err_once("TME: configuation is
> > inconsistent between CPUs\n");
> > +			mktme_status = MKTME_DISABLED;
> > +		}
> > +		goto out;
> 
> Why goto out here? If something goes wrong, I think it is pointless to
> read keyID bits staff? IMHO if something goes wrong, you should set
> mktme_status to disabled, and clear tme_activate_cpu0?

We still have to mask out keyid bits from x86_phys_bits if CPU has TME
enabled. But yeah, as you pointed below, I need to check that it actually
locked and enabled.

> > +	}
> > +
> > +	tme_activate_cpu0 = tme_activate;
> > +
> > +	if (!TME_ACTIVATE_LOCKED(tme_activate) ||
> > !TME_ACTIVATE_ENABLED(tme_activate)) {
> > +		pr_info("TME: not enabled by BIOS\n");
> > +		mktme_status = MKTME_DISABLED;
> > +		goto out;
> 
> I think it is pointless to read keyID bits staff if TME is not even
> enabled.
> 
> > +	}
> > +
> > +	pr_info("TME: enabled by BIOS\n");
> > +
> > +	tme_policy = TME_ACTIVATE_POLICY(tme_activate);
> > +	if (tme_policy != TME_ACTIVATE_POLICY_AES_XTS)
> > +		pr_warn("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)) {
> > +		pr_err("MKTME: No known encryption algorithm is
> > supported: %#llx\n",
> > +				tme_crypto_algs);
> > +		mktme_status = MKTME_DISABLED;
> > +	}
> 
> To me it is a little bit confusing about the naming. tme_policy is the
> crypto_alg used by TME keyID (0), and tme_crypto_algs is bitmap of
> supported crypto_algs for MK-TME. Probably a better naming is needed?
> And the naming of TME_ACTIVATE_POLICY(x), TME_ACTIVATE_CRYPTO_ALGS(x)
> above as well?

Suggestions?

-- 
 Kirill A. Shutemov

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

[Kai Huang]

On Wed, 2018-02-07 at 11:16 +0300, Kirill A. Shutemov wrote:
> On Wed, Feb 07, 2018 at 05:34:10PM +1300, Kai Huang wrote:
> > On Wed, 2018-01-31 at 12:15 +0300, 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.
> > > 
> > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.co
> > > m>
> > > ---
> > >  arch/x86/kernel/cpu/intel.c | 83
> > > +++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 83 insertions(+)
> > > 
> > > diff --git a/arch/x86/kernel/cpu/intel.c
> > > b/arch/x86/kernel/cpu/intel.c
> > > index 6936d14d4c77..5b95fa484837 100644
> > > --- a/arch/x86/kernel/cpu/intel.c
> > > +++ b/arch/x86/kernel/cpu/intel.c
> > > @@ -517,6 +517,86 @@ static void detect_vmx_virtcap(struct
> > > cpuinfo_x86 *c)
> > >  	}
> > >  }
> > >  
> > > +#define MSR_IA32_TME_ACTIVATE		0x982
> > 
> > Should this MSR go into msr-index.h?
> 
> No. Comment from msr-index.h:
> 
>  * Do not add new entries to this file unless the definitions are
> shared
>  * between multiple compilation units.
> 
> > > +
> > > +#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	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	1
> > > +
> > > +#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;
> > > +
> > > +	rdmsrl(MSR_IA32_TME_ACTIVATE, tme_activate);
> > > +
> > > +	if (mktme_status != MKTME_UNINITIALIZED) {
> > > +		/* Broken BIOS? */
> > > +		if (tme_activate != tme_activate_cpu0) {
> > > +			pr_err_once("TME: configuation is
> > > inconsistent between CPUs\n");
> > > +			mktme_status = MKTME_DISABLED;
> > > +		}
> > > +		goto out;
> > 
> > Why goto out here? If something goes wrong, I think it is pointless
> > to
> > read keyID bits staff? IMHO if something goes wrong, you should set
> > mktme_status to disabled, and clear tme_activate_cpu0?
> 
> We still have to mask out keyid bits from x86_phys_bits if CPU has
> TME
> enabled. 

Reading spec again yes you are right.

> But yeah, as you pointed below, I need to check that it actually
> locked and enabled.
> 
> > > +	}
> > > +
> > > +	tme_activate_cpu0 = tme_activate;
> > > +
> > > +	if (!TME_ACTIVATE_LOCKED(tme_activate) ||
> > > !TME_ACTIVATE_ENABLED(tme_activate)) {
> > > +		pr_info("TME: not enabled by BIOS\n");
> > > +		mktme_status = MKTME_DISABLED;
> > > +		goto out;
> > 
> > I think it is pointless to read keyID bits staff if TME is not even
> > enabled.
> > 
> > > +	}
> > > +
> > > +	pr_info("TME: enabled by BIOS\n");
> > > +
> > > +	tme_policy = TME_ACTIVATE_POLICY(tme_activate);
> > > +	if (tme_policy != TME_ACTIVATE_POLICY_AES_XTS)
> > > +		pr_warn("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)) {
> > > +		pr_err("MKTME: No known encryption algorithm is
> > > supported: %#llx\n",
> > > +				tme_crypto_algs);
> > > +		mktme_status = MKTME_DISABLED;
> > > +	}
> > 
> > To me it is a little bit confusing about the naming. tme_policy is
> > the
> > crypto_alg used by TME keyID (0), and tme_crypto_algs is bitmap of
> > supported crypto_algs for MK-TME. Probably a better naming is
> > needed?
> > And the naming of TME_ACTIVATE_POLICY(x),
> > TME_ACTIVATE_CRYPTO_ALGS(x)
> > above as well?
> 
> Suggestions?

I would go with 'tme_cryto_alg', and 'mktme_supported_crypto_algs' for
the two variables, and TME_CRYPTO_ALG(x), TME_CRYPTO_ALG_AES_XTS_128, 
MKTME_SUPPORTED_CRYPTO_ALGS(x), and MKTME_CRYPTO_ALG_AES_XTS_128 for
the macros, but it's up to you and other guys.

Btw I do think AES_XTS should be AES_XTS_128, even you go with current
names, as AES cipher can also take 256-bit key.

Thanks,
-Kai
> 

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

[Kirill A. Shutemov]

On Thu, Feb 08, 2018 at 12:00:44AM +1300, Kai Huang wrote:
> > Suggestions?
> 
> I would go with 'tme_cryto_alg', and 'mktme_supported_crypto_algs' for
> the two variables, and TME_CRYPTO_ALG(x), TME_CRYPTO_ALG_AES_XTS_128, 
> MKTME_SUPPORTED_CRYPTO_ALGS(x), and MKTME_CRYPTO_ALG_AES_XTS_128 for
> the macros, but it's up to you and other guys.

I'll leave it as is for now.

> Btw I do think AES_XTS should be AES_XTS_128, even you go with current
> names, as AES cipher can also take 256-bit key.

Good point.

-- 
 Kirill A. Shutemov