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?
