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.shute...@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);