| Message ID | 20241127201019.136086-4-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: VMX: Initialize TDX during KVM module load | expand |
>+static void __do_tdx_cleanup(void) >+{ >+ /* >+ * Once TDX module is initialized, it cannot be disabled and >+ * re-initialized again w/o runtime update (which isn't >+ * supported by kernel). Only need to remove the cpuhp here. >+ * The TDX host core code tracks TDX status and can handle >+ * 'multiple enabling' scenario. >+ */ >+ WARN_ON_ONCE(!tdx_cpuhp_state); >+ cpuhp_remove_state_nocalls(tdx_cpuhp_state); ... >+static int __init __do_tdx_bringup(void) >+{ >+ int r; >+ >+ /* >+ * TDX-specific cpuhp callback to call tdx_cpu_enable() on all >+ * online CPUs before calling tdx_enable(), and on any new >+ * going-online CPU to make sure it is ready for TDX guest. >+ */ >+ r = cpuhp_setup_state_cpuslocked(CPUHP_AP_ONLINE_DYN, >+ "kvm/cpu/tdx:online", >+ tdx_online_cpu, NULL); >+ if (r < 0) >+ return r; >+ >+ tdx_cpuhp_state = r; >+ >+ r = tdx_enable(); >+ if (r) >+ __do_tdx_cleanup(); The self deadlock issue isn't addressed. >+ >+ return r; >+} >+ >+static bool __init kvm_can_support_tdx(void) >+{ >+ return cpu_feature_enabled(X86_FEATURE_TDX_HOST_PLATFORM); ... >+int __init tdx_bringup(void) >+{ >+ int r; >+ >+ if (!enable_tdx) >+ return 0; >+ >+ if (!kvm_can_support_tdx()) { >+ pr_err("tdx: no TDX private KeyIDs available\n"); The lack of X86_FEATURE_TDX_HOST_PLATFORM doesn't necessarily mean no TDX private KeyIDs. In tdx_init(), X86_FEATURE_TDX_HOST_PLATFORM is not set if hibernation is enabled. >+ goto success_disable_tdx; >+ } >+ >+ if (!enable_virt_at_load) { >+ pr_err("tdx: tdx requires kvm.enable_virt_at_load=1\n"); >+ goto success_disable_tdx; >+ } >+ >+ /* >+ * Ideally KVM should probe whether TDX module has been loaded >+ * first and then try to bring it up. But TDX needs to use SEAMCALL >+ * to probe whether the module is loaded (there is no CPUID or MSR >+ * for that), and making SEAMCALL requires enabling virtualization >+ * first, just like the rest steps of bringing up TDX module. >+ * >+ * So, for simplicity do everything in __tdx_bringup(); the first >+ * SEAMCALL will return -ENODEV when the module is not loaded. The >+ * only complication is having to make sure that initialization >+ * SEAMCALLs don't return TDX_SEAMCALL_VMFAILINVALID in other >+ * cases. >+ */ >+ r = __tdx_bringup(); >+ if (r) { >+ /* >+ * Disable TDX only but don't fail to load module if >+ * the TDX module could not be loaded. No need to print >+ * message saying "module is not loaded" because it was >+ * printed when the first SEAMCALL failed. >+ */ >+ if (r == -ENODEV) >+ goto success_disable_tdx; Given that two error messages are added above, I think it is worth adding one more here for consistency. And, reloading KVM will not trigger the "module is not loaded" error which is printed once. >+ >+ enable_tdx = 0; >+ } >+ >+ return r; >+ >+success_disable_tdx: >+ enable_tdx = 0; >+ return 0; >+}
> >+ r = tdx_enable(); > >+ if (r) > >+ __do_tdx_cleanup(); > > The self deadlock issue isn't addressed. > I planned to send out a new version after merge window, but Paolo beats me. I have fixed and now testing. I'll send out the fixup soon. Thanks Paolo!
Hi Paolo, Thanks for doing this! > + > +static void __do_tdx_cleanup(void) > +{ > + /* > + * Once TDX module is initialized, it cannot be disabled and > + * re-initialized again w/o runtime update (which isn't > + * supported by kernel). Only need to remove the cpuhp here. > + * The TDX host core code tracks TDX status and can handle > + * 'multiple enabling' scenario. > + */ > + WARN_ON_ONCE(!tdx_cpuhp_state); > + cpuhp_remove_state_nocalls(tdx_cpuhp_state); > + tdx_cpuhp_state = 0; > +} As Gao, Chao pointed out, there's bug here since it is also called by __do_tdx_bringup() which is called with cpus_read_lock() hold. We need the _cpuslocked() version here. I pasted the fixup at the bottom of this reply for your reference and please merge if it is fine to you. The diff is also attached if that's easier. [...] > +int __init tdx_bringup(void) > +{ > + int r; > + > + if (!enable_tdx) > + return 0; > + > + if (!kvm_can_support_tdx()) { > + pr_err("tdx: no TDX private KeyIDs available\n"); > + goto success_disable_tdx; > + } > + > + if (!enable_virt_at_load) { > + pr_err("tdx: tdx requires kvm.enable_virt_at_load=1\n"); > + goto success_disable_tdx; > + } The intention of kvm_can_support_tdx() is to put all dependency checks to it. I think we should just put the check of 'enable_virt_at_load' inside. And there will be more checks in the later patches such as checking 'tdp_mmu_enabled' and 'enable_mmio_caching' so it doesn't make sense to check 'enable_virt_at_load' outside. diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c index 0666dfbe0bc0..d8c008437e79 100644 --- a/arch/x86/kvm/vmx/tdx.c +++ b/arch/x86/kvm/vmx/tdx.c @@ -38,10 +38,17 @@ static void __do_tdx_cleanup(void) * 'multiple enabling' scenario. */ WARN_ON_ONCE(!tdx_cpuhp_state); - cpuhp_remove_state_nocalls(tdx_cpuhp_state); + cpuhp_remove_state_nocalls_cpuslocked(tdx_cpuhp_state); tdx_cpuhp_state = 0; } +static void __tdx_cleanup(void) +{ + cpus_read_lock(); + __do_tdx_cleanup(); + cpus_read_unlock(); +} + static int __init __do_tdx_bringup(void) { int r; @@ -68,7 +75,17 @@ static int __init __do_tdx_bringup(void) static bool __init kvm_can_support_tdx(void) { - return cpu_feature_enabled(X86_FEATURE_TDX_HOST_PLATFORM); + if (!cpu_feature_enabled(X86_FEATURE_TDX_HOST_PLATFORM)) { + pr_err("tdx: no TDX private KeyIDs available\n"); + return false; + } + + if (!enable_virt_at_load) { + pr_err("tdx: tdx requires kvm.enable_virt_at_load=1\n"); + return false; + } + + return true; } static int __init __tdx_bringup(void) @@ -103,7 +120,7 @@ static int __init __tdx_bringup(void) void tdx_cleanup(void) { if (enable_tdx) { - __do_tdx_cleanup(); + __tdx_cleanup(); kvm_disable_virtualization(); } } @@ -115,15 +132,8 @@ int __init tdx_bringup(void) if (!enable_tdx) return 0; - if (!kvm_can_support_tdx()) { - pr_err("tdx: no TDX private KeyIDs available\n"); - goto success_disable_tdx; - } - - if (!enable_virt_at_load) { - pr_err("tdx: tdx requires kvm.enable_virt_at_load=1\n"); + if (!kvm_can_support_tdx()) goto success_disable_tdx; - } /* * Ideally KVM should probe whether TDX module has been loaded
diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig index ea2c4f21c1ca..fe8cbee6f614 100644 --- a/arch/x86/kvm/Kconfig +++ b/arch/x86/kvm/Kconfig @@ -128,6 +128,16 @@ config X86_SGX_KVM If unsure, say N. +config KVM_INTEL_TDX + bool "Intel Trust Domain Extensions (TDX) support" + default y + depends on INTEL_TDX_HOST + help + Provides support for launching Intel Trust Domain Extensions (TDX) + confidential VMs on Intel processors. + + If unsure, say N. + config KVM_AMD tristate "KVM for AMD processors support" depends on KVM && (CPU_SUP_AMD || CPU_SUP_HYGON) diff --git a/arch/x86/kvm/Makefile b/arch/x86/kvm/Makefile index f9dddb8cb466..a5d362c7b504 100644 --- a/arch/x86/kvm/Makefile +++ b/arch/x86/kvm/Makefile @@ -20,6 +20,7 @@ kvm-intel-y += vmx/vmx.o vmx/vmenter.o vmx/pmu_intel.o vmx/vmcs12.o \ kvm-intel-$(CONFIG_X86_SGX_KVM) += vmx/sgx.o kvm-intel-$(CONFIG_KVM_HYPERV) += vmx/hyperv.o vmx/hyperv_evmcs.o +kvm-intel-$(CONFIG_KVM_INTEL_TDX) += vmx/tdx.o kvm-amd-y += svm/svm.o svm/vmenter.o svm/pmu.o svm/nested.o svm/avic.o diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c index 6772e560ac7b..bc690c63e511 100644 --- a/arch/x86/kvm/vmx/main.c +++ b/arch/x86/kvm/vmx/main.c @@ -6,6 +6,7 @@ #include "nested.h" #include "pmu.h" #include "posted_intr.h" +#include "tdx.h" #define VMX_REQUIRED_APICV_INHIBITS \ (BIT(APICV_INHIBIT_REASON_DISABLED) | \ @@ -171,6 +172,7 @@ struct kvm_x86_init_ops vt_init_ops __initdata = { static void vt_exit(void) { kvm_exit(); + tdx_cleanup(); vmx_exit(); } module_exit(vt_exit); @@ -183,6 +185,11 @@ static int __init vt_init(void) if (r) return r; + /* tdx_init() has been taken */ + r = tdx_bringup(); + if (r) + goto err_tdx_bringup; + /* * Common KVM initialization _must_ come last, after this, /dev/kvm is * exposed to userspace! @@ -195,6 +202,8 @@ static int __init vt_init(void) return 0; err_kvm_init: + tdx_cleanup(); +err_tdx_bringup: vmx_exit(); return r; } diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c new file mode 100644 index 000000000000..0666dfbe0bc0 --- /dev/null +++ b/arch/x86/kvm/vmx/tdx.c @@ -0,0 +1,160 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <linux/cpu.h> +#include <asm/cpufeature.h> +#include <asm/tdx.h> +#include "capabilities.h" +#include "tdx.h" + +#undef pr_fmt +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +static bool enable_tdx __ro_after_init; +module_param_named(tdx, enable_tdx, bool, 0444); + +static enum cpuhp_state tdx_cpuhp_state; + +static int tdx_online_cpu(unsigned int cpu) +{ + unsigned long flags; + int r; + + /* Sanity check CPU is already in post-VMXON */ + WARN_ON_ONCE(!(cr4_read_shadow() & X86_CR4_VMXE)); + + local_irq_save(flags); + r = tdx_cpu_enable(); + local_irq_restore(flags); + + return r; +} + +static void __do_tdx_cleanup(void) +{ + /* + * Once TDX module is initialized, it cannot be disabled and + * re-initialized again w/o runtime update (which isn't + * supported by kernel). Only need to remove the cpuhp here. + * The TDX host core code tracks TDX status and can handle + * 'multiple enabling' scenario. + */ + WARN_ON_ONCE(!tdx_cpuhp_state); + cpuhp_remove_state_nocalls(tdx_cpuhp_state); + tdx_cpuhp_state = 0; +} + +static int __init __do_tdx_bringup(void) +{ + int r; + + /* + * TDX-specific cpuhp callback to call tdx_cpu_enable() on all + * online CPUs before calling tdx_enable(), and on any new + * going-online CPU to make sure it is ready for TDX guest. + */ + r = cpuhp_setup_state_cpuslocked(CPUHP_AP_ONLINE_DYN, + "kvm/cpu/tdx:online", + tdx_online_cpu, NULL); + if (r < 0) + return r; + + tdx_cpuhp_state = r; + + r = tdx_enable(); + if (r) + __do_tdx_cleanup(); + + return r; +} + +static bool __init kvm_can_support_tdx(void) +{ + return cpu_feature_enabled(X86_FEATURE_TDX_HOST_PLATFORM); +} + +static int __init __tdx_bringup(void) +{ + int r; + + /* + * Enabling TDX requires enabling hardware virtualization first, + * as making SEAMCALLs requires CPU being in post-VMXON state. + */ + r = kvm_enable_virtualization(); + if (r) + return r; + + cpus_read_lock(); + r = __do_tdx_bringup(); + cpus_read_unlock(); + + if (r) + goto tdx_bringup_err; + + /* + * Leave hardware virtualization enabled after TDX is enabled + * successfully. TDX CPU hotplug depends on this. + */ + return 0; +tdx_bringup_err: + kvm_disable_virtualization(); + return r; +} + +void tdx_cleanup(void) +{ + if (enable_tdx) { + __do_tdx_cleanup(); + kvm_disable_virtualization(); + } +} + +int __init tdx_bringup(void) +{ + int r; + + if (!enable_tdx) + return 0; + + if (!kvm_can_support_tdx()) { + pr_err("tdx: no TDX private KeyIDs available\n"); + goto success_disable_tdx; + } + + if (!enable_virt_at_load) { + pr_err("tdx: tdx requires kvm.enable_virt_at_load=1\n"); + goto success_disable_tdx; + } + + /* + * Ideally KVM should probe whether TDX module has been loaded + * first and then try to bring it up. But TDX needs to use SEAMCALL + * to probe whether the module is loaded (there is no CPUID or MSR + * for that), and making SEAMCALL requires enabling virtualization + * first, just like the rest steps of bringing up TDX module. + * + * So, for simplicity do everything in __tdx_bringup(); the first + * SEAMCALL will return -ENODEV when the module is not loaded. The + * only complication is having to make sure that initialization + * SEAMCALLs don't return TDX_SEAMCALL_VMFAILINVALID in other + * cases. + */ + r = __tdx_bringup(); + if (r) { + /* + * Disable TDX only but don't fail to load module if + * the TDX module could not be loaded. No need to print + * message saying "module is not loaded" because it was + * printed when the first SEAMCALL failed. + */ + if (r == -ENODEV) + goto success_disable_tdx; + + enable_tdx = 0; + } + + return r; + +success_disable_tdx: + enable_tdx = 0; + return 0; +} diff --git a/arch/x86/kvm/vmx/tdx.h b/arch/x86/kvm/vmx/tdx.h new file mode 100644 index 000000000000..8aee938a968f --- /dev/null +++ b/arch/x86/kvm/vmx/tdx.h @@ -0,0 +1,13 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef __KVM_X86_VMX_TDX_H +#define __KVM_X86_VMX_TDX_H + +#ifdef CONFIG_INTEL_TDX_HOST +int tdx_bringup(void); +void tdx_cleanup(void); +#else +static inline int tdx_bringup(void) { return 0; } +static inline void tdx_cleanup(void) {} +#endif + +#endif diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 3fcee4209120..0a141685872d 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -2272,6 +2272,7 @@ static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu) } #ifdef CONFIG_KVM_GENERIC_HARDWARE_ENABLING +extern bool enable_virt_at_load; extern bool kvm_rebooting; #endif diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 8214efa76a6f..265b687fc5d6 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -5440,8 +5440,9 @@ static struct miscdevice kvm_dev = { }; #ifdef CONFIG_KVM_GENERIC_HARDWARE_ENABLING -static bool enable_virt_at_load = true; +bool enable_virt_at_load = true; module_param(enable_virt_at_load, bool, 0444); +EXPORT_SYMBOL_GPL(enable_virt_at_load); __visible bool kvm_rebooting; EXPORT_SYMBOL_GPL(kvm_rebooting);