lkml.org 
[lkml]   [2022]   [Jun]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v5 01/22] x86/virt/tdx: Detect TDX during kernel boot
    On Wed, Jun 22, 2022 at 11:15:30PM +1200, Kai Huang wrote:
    >Intel Trust Domain Extensions (TDX) protects guest VMs from malicious
    >host and certain physical attacks. TDX introduces a new CPU mode called
    >Secure Arbitration Mode (SEAM) and a new isolated range pointed by the
    ^ perhaps, range of memory

    >SEAM Ranger Register (SEAMRR). A CPU-attested software module called
    >'the TDX module' runs inside the new isolated range to implement the
    >functionalities to manage and run protected VMs.
    >
    >Pre-TDX Intel hardware has support for a memory encryption architecture
    >called MKTME. The memory encryption hardware underpinning MKTME is also
    >used for Intel TDX. TDX ends up "stealing" some of the physical address
    >space from the MKTME architecture for crypto-protection to VMs. BIOS is
    >responsible for partitioning the "KeyID" space between legacy MKTME and
    >TDX. The KeyIDs reserved for TDX are called 'TDX private KeyIDs' or
    >'TDX KeyIDs' for short.
    >
    >To enable TDX, BIOS needs to configure SEAMRR (core-scope) and TDX
    >private KeyIDs (package-scope) consistently for all packages. TDX
    >doesn't trust BIOS. TDX ensures all BIOS configurations are correct,
    >and if not, refuses to enable SEAMRR on any core. This means detecting
    >SEAMRR alone on BSP is enough to check whether TDX has been enabled by
    >BIOS.
    >
    >To start to support TDX, create a new arch/x86/virt/vmx/tdx/tdx.c for
    >TDX host kernel support. Add a new Kconfig option CONFIG_INTEL_TDX_HOST
    >to opt-in TDX host kernel support (to distinguish with TDX guest kernel
    >support). So far only KVM is the only user of TDX. Make the new config
    >option depend on KVM_INTEL.
    >
    >Use early_initcall() to detect whether TDX is enabled by BIOS during
    >kernel boot, and add a function to report that. Use a function instead
    >of a new CPU feature bit. This is because the TDX module needs to be
    >initialized before it can be used to run any TDX guests, and the TDX
    >module is initialized at runtime by the caller who wants to use TDX.
    >
    >Explicitly detect SEAMRR but not just only detect TDX private KeyIDs.
    >Theoretically, a misconfiguration of TDX private KeyIDs can result in
    >SEAMRR being disabled, but the BSP can still report the correct TDX
    >KeyIDs. Such BIOS bug can be caught when initializing the TDX module,
    >but it's better to do more detection during boot to provide a more
    >accurate result.
    >
    >Also detect the TDX KeyIDs. This allows userspace to know how many TDX
    >guests the platform can run w/o needing to wait until TDX is fully
    >functional.
    >
    >Signed-off-by: Kai Huang <kai.huang@intel.com>

    Reviewed-by: Chao Gao <chao.gao@intel.com>

    But some cosmetic comments below ...

    >---
    >+
    >+static u32 tdx_keyid_start __ro_after_init;
    >+static u32 tdx_keyid_num __ro_after_init;
    >+
    ...

    >+static int detect_tdx_keyids(void)
    >+{
    >+ u64 keyid_part;
    >+
    >+ rdmsrl(MSR_IA32_MKTME_KEYID_PARTITIONING, keyid_part);

    how about:
    rdmsr(MSR_IA32_MKTME_KEYID_PARTITIONING, tdx_keyid_start, tdx_keyid_num);
    tdx_keyid_start++;

    Then TDX_KEYID_NUM/START can be dropped.

    >+
    >+ tdx_keyid_num = TDX_KEYID_NUM(keyid_part);
    >+ tdx_keyid_start = TDX_KEYID_START(keyid_part);
    >+
    >+ pr_info("TDX private KeyID range: [%u, %u).\n",
    >+ tdx_keyid_start, tdx_keyid_start + tdx_keyid_num);
    >+
    >+ /*
    >+ * TDX guarantees at least two TDX KeyIDs are configured by
    >+ * BIOS, otherwise SEAMRR is disabled. Invalid TDX private
    >+ * range means kernel bug (TDX is broken).

    Maybe it is better to have a comment for why TDX/kernel guarantees
    there should be at least 2 TDX keyIDs.

    >+
    >+/*
    >+ * This file contains both macros and data structures defined by the TDX
    >+ * architecture and Linux defined software data structures and functions.
    >+ * The two should not be mixed together for better readability. The
    >+ * architectural definitions come first.
    >+ */
    >+
    >+/*
    >+ * Intel Trusted Domain CPU Architecture Extension spec:
    >+ *
    >+ * IA32_MTRRCAP:
    >+ * Bit 15: The support of SEAMRR
    >+ *
    >+ * IA32_SEAMRR_PHYS_MASK (core-scope):
    >+ * Bit 10: Lock bit
    >+ * Bit 11: Enable bit
    >+ */
    >+#define MTRR_CAP_SEAMRR BIT_ULL(15)

    Can you move this bit definition to arch/x86/include/asm/msr-index.h
    right after MSR_MTRRcap definition there?

    \
     
     \ /
      Last update: 2022-06-23 07:59    [W:4.398 / U:0.136 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site