| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <36aac3cb7c7447db6454ee396e25eea3bad378e6.camel@intel.com>
Date: Thu, 31 Mar 2022 16:31:10 +1300
From: Kai Huang <kai.huang@...el.com>
To: isaku.yamahata@...el.com, kvm@...r.kernel.org,
linux-kernel@...r.kernel.org
Cc: isaku.yamahata@...il.com, Paolo Bonzini <pbonzini@...hat.com>,
Jim Mattson <jmattson@...gle.com>, erdemaktas@...gle.com,
Connor Kuehl <ckuehl@...hat.com>,
Sean Christopherson <seanjc@...gle.com>
Subject: Re: [RFC PATCH v5 008/104] KVM: TDX: Add a function to initialize
TDX module
On Fri, 2022-03-04 at 11:48 -0800, isaku.yamahata@...el.com wrote:
> From: Isaku Yamahata <isaku.yamahata@...el.com>
>
> Memory used for TDX is encrypted with an encryption key. An encryption key
> is assigned to guest TD, and TDX memory is encrypted. VMM calculates Trust
> Domain Memory Range (TDMR), a range of memory pages that can hold TDX
> memory encrypted with an encryption key. VMM allocates memory regions for
> Physical Address Metadata Table (PAMT) which the TDX module uses to track
> page states. Used for TDX memory, assigned to which guest TD, etc. VMM
> gives PAMT regions to the TDX module and initializes it which is also
> encrypted.
Not sure why above are related to this patch. Perhaps you can just say TDX
module is detected and initialized on demand via tdx_detect() and tdx_init().
>
> TDX requires more initialization steps in addition to VMX. As a
> preparation step, check if the CPU feature is available and enable VMX
> because the TDX module API requires VMX to be enabled to be functional.
Those are not reflected in this patch either.
> The next step is basic platform initialization. Check if TDX module API is
> available, call system-wide initialization API (TDH.SYS.INIT), and call LP
> initialization API (TDH.SYS.LP.INIT). Lastly, get system-wide
> parameters (TDH.SYS.INFO), allocate PAMT for TDX module to track page
> states (TDH.SYS.CONFIG), configure encryption key (TDH.SYS.KEY.CONFIG), and
> initialize PAMT (TDH.SYS.TDMR.INIT).
Again, not sure why those are related.
>
> A TDX host patch series implements those details and it provides APIs,
> seamrr_enabled() to check if CPU feature is available, init_tdx() to
> initialize the TDX module, tdx_get_tdsysinfo() to get TDX system
> parameters.
init_tdx() -> tdx_init().
"A TDX host patch series" should not be in the formal commit message, I suppose.
>
> Add a wrapper function to initialize the TDX module and get system-wide
> parameters via those APIs. Because TDX requires VMX enabled, It will be
> called on-demand when the first guest TD is created via x86 KVM init_vm
> callback.
Why not just merge this patch with the change where you implement the init_vm
callback? Then you can just declare this patch as "detect and initialize TDX
module when first VM is created", or something like that..
>
> Signed-off-by: Isaku Yamahata <isaku.yamahata@...el.com>
> ---
> arch/x86/kvm/vmx/tdx.c | 89 ++++++++++++++++++++++++++++++++++++++++++
> arch/x86/kvm/vmx/tdx.h | 4 ++
> 2 files changed, 93 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index 8ed3ec342e28..8adc87ad1807 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -13,9 +13,98 @@
> static bool __read_mostly enable_tdx = true;
> module_param_named(tdx, enable_tdx, bool, 0644);
>
> +#define TDX_MAX_NR_CPUID_CONFIGS \
> + ((sizeof(struct tdsysinfo_struct) - \
> + offsetof(struct tdsysinfo_struct, cpuid_configs)) \
> + / sizeof(struct tdx_cpuid_config))
> +
> +struct tdx_capabilities {
> + u8 tdcs_nr_pages;
> + u8 tdvpx_nr_pages;
> +
> + u64 attrs_fixed0;
> + u64 attrs_fixed1;
> + u64 xfam_fixed0;
> + u64 xfam_fixed1;
> +
> + u32 nr_cpuid_configs;
> + struct tdx_cpuid_config cpuid_configs[TDX_MAX_NR_CPUID_CONFIGS];
> +};
> +
> +/* Capabilities of KVM + the TDX module. */
> +struct tdx_capabilities tdx_caps;
> +
> static u64 hkid_mask __ro_after_init;
> static u8 hkid_start_pos __ro_after_init;
The two seems are not used in this patch.
Please make sure each patch can compile w/o warning.
>
> +static int __tdx_module_setup(void)
> +{
> + const struct tdsysinfo_struct *tdsysinfo;
> + int ret = 0;
> +
> + BUILD_BUG_ON(sizeof(*tdsysinfo) != 1024);
> + BUILD_BUG_ON(TDX_MAX_NR_CPUID_CONFIGS != 37);
> +
> + ret = tdx_detect();
> + if (ret) {
> + pr_info("Failed to detect TDX module.\n");
> + return ret;
> + }
> +
> + ret = tdx_init();
> + if (ret) {
> + pr_info("Failed to initialize TDX module.\n");
> + return ret;
> + }
> +
> + tdsysinfo = tdx_get_sysinfo();
> + if (tdx_caps.nr_cpuid_configs > TDX_MAX_NR_CPUID_CONFIGS)
> + return -EIO;
> +
> + tdx_caps = (struct tdx_capabilities) {
> + .tdcs_nr_pages = tdsysinfo->tdcs_base_size / PAGE_SIZE,
> + /*
> + * TDVPS = TDVPR(4K page) + TDVPX(multiple 4K pages).
> + * -1 for TDVPR.
> + */
> + .tdvpx_nr_pages = tdsysinfo->tdvps_base_size / PAGE_SIZE - 1,
> + .attrs_fixed0 = tdsysinfo->attributes_fixed0,
> + .attrs_fixed1 = tdsysinfo->attributes_fixed1,
> + .xfam_fixed0 = tdsysinfo->xfam_fixed0,
> + .xfam_fixed1 = tdsysinfo->xfam_fixed1,
> + .nr_cpuid_configs = tdsysinfo->num_cpuid_config,
> + };
> + if (!memcpy(tdx_caps.cpuid_configs, tdsysinfo->cpuid_configs,
> + tdsysinfo->num_cpuid_config *
> + sizeof(struct tdx_cpuid_config)))
> + return -EIO;
> +
> + return 0;
> +}
> +
> +int tdx_module_setup(void)
> +{
> + static DEFINE_MUTEX(tdx_init_lock);
> + static bool __read_mostly tdx_module_initialized;
> + int ret = 0;
> +
> + mutex_lock(&tdx_init_lock);
It took me a while to figure out why this mutex is needed. Please see my above
suggestion to merge this patch to the change that implements init_vm() callback.
> +
> + if (!tdx_module_initialized) {
> + if (enable_tdx) {
> + ret = __tdx_module_setup();
I think you can move tdx_detect() and tdx_init() out of your mutex. They are
internally protected by mutex.
> + if (ret)
> + enable_tdx = false;
> + else
> + tdx_module_initialized = true;
> + } else
> + ret = -EOPNOTSUPP;
> + }
> +
> + mutex_unlock(&tdx_init_lock);
> + return ret;
> +}
> +
> static int __init __tdx_hardware_setup(struct kvm_x86_ops *x86_ops)
> {
> u32 max_pa;
> diff --git a/arch/x86/kvm/vmx/tdx.h b/arch/x86/kvm/vmx/tdx.h
> index daf6bfc6502a..d448e019602c 100644
> --- a/arch/x86/kvm/vmx/tdx.h
> +++ b/arch/x86/kvm/vmx/tdx.h
> @@ -3,6 +3,8 @@
> #define __KVM_X86_TDX_H
>
> #ifdef CONFIG_INTEL_TDX_HOST
> +int tdx_module_setup(void);
> +
> struct kvm_tdx {
> struct kvm kvm;
> };
> @@ -35,6 +37,8 @@ static inline struct vcpu_tdx *to_tdx(struct kvm_vcpu *vcpu)
> return container_of(vcpu, struct vcpu_tdx, vcpu);
> }
> #else
> +static inline int tdx_module_setup(void) { return -ENODEV; };
> +
> struct kvm_tdx;
> struct vcpu_tdx;
>
Powered by blists - more mailing lists