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/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
