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

Reply via email to