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.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?
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