Re: [PATCH v14 004/113] KVM: TDX: Initialize the TDX module when loading the KVM intel kernel module

[Isaku Yamahata]

On Tue, Jun 06, 2023 at 04:19:33AM +0000,
"Huang, Kai" <kai.huang@xxxxxxxxx> wrote:

> On Sun, 2023-05-28 at 21:18 -0700, Yamahata, Isaku wrote:
> > +static void __init vmx_tdx_on(void *info)
> > +{
> > +       atomic_t *err = info;
> > +       int r;
> > +
> > +       r = vmx_hardware_enable();
> > +       if (!r)
> > +	       r = tdx_cpu_enable();
> > +       if (r)
> > +	       atomic_set(err, r);
> > +}
> > +
> > +static void __init vmx_off(void *unused)
> > +{
> > +       vmx_hardware_disable();
> > +}
> > +
> > +int __init tdx_hardware_setup(struct kvm_x86_ops *x86_ops)
> > +{
> > +	atomic_t err = ATOMIC_INIT(0);
> > +	int r = 0;
> > +
> > +	if (!enable_ept) {
> > +		pr_warn("Cannot enable TDX with EPT disabled\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* tdx_enable() in tdx_module_setup() requires cpus lock. */
> > +	cpus_read_lock();
> > +	on_each_cpu(vmx_tdx_on, &err, true);	/* TDX requires vmxon. */
> > +	r = atomic_read(&err);
> > +	if (!r)
> > +		r = tdx_module_setup();
> > +	on_each_cpu(vmx_off, NULL, true);
> > +	cpus_read_unlock();
> > +
> > +	return r;
> > +}
> 
> As we discussed in v13, this code doesn't track which CPUs have run
> vmx_hardware_enable() successfully.  Thus if ...
> 
> 	on_each_cpu(vmx_tdx_on, &err, true);	/* TDX requires vmxon. */
> 
> ... fails on some cpu due to whatever reason, in ...
> 	
> 	on_each_cpu(vmx_off, NULL, true);
> 
> ... vmx_hardware_disable() will fail to do VMXOFF for those cpus that haven't
> done VMXON successfully yet, resulting in BUG_ON(!kvm_rebooting) being triggered
> in kvm_spurious_fault().
> 
> We need a per-cpu flag to track whether cpu has done VMXON successfully.

Thanks for pointing it out. The following is the fix.

diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 8a1d0755d275..b0d3f646afb1 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -4499,26 +4499,39 @@ u64 tdx_non_arch_field_switch(u64 field)
 	}
 }
 
-static void __init vmx_tdx_on(void *info)
+struct vmx_tdx_enabled {
+	cpumask_var_t vmx_enabled;
+	atomic_t *err;
+};
+
+static void __init vmx_tdx_on(void *_vmx_tdx_on)
 {
-	atomic_t *err = info;
+	struct vmx_tdx_enabled *vmx_tdx = _vmx_tdx_on;
 	int r;
 
 	r = vmx_hardware_enable();
-	if (!r)
+	if (!r) {
+		cpumask_set_cpu(smp_processor_id(), vmx_tdx->vmx_enabled);
 		r = tdx_cpu_enable();
+	}
 	if (r)
-		atomic_set(err, r);
+		atomic_set(vmx_tdx->err, r);
 }
 
-static void __init vmx_off(void *unused)
+static void __init vmx_off(void *_vmx_enabled)
 {
-	vmx_hardware_disable();
+	cpumask_var_t vmx_enabled = *(cpumask_var_t *)_vmx_enabled;
+
+	if (cpumask_test_cpu(smp_processor_id(), vmx_enabled))
+		vmx_hardware_disable();
 }
 
 int __init tdx_hardware_setup(struct kvm_x86_ops *x86_ops)
 {
 	atomic_t err = ATOMIC_INIT(0);
+	struct vmx_tdx_enabled vmx_tdx = {
+		.err = &err,
+	};
 	int max_pkgs;
 	int r = 0;
 	int i;
@@ -4582,6 +4595,11 @@ int __init tdx_hardware_setup(struct kvm_x86_ops *x86_ops)
 	for (i = 0; i < max_pkgs; i++)
 		mutex_init(&tdx_mng_key_config_lock[i]);
 
+	if (!zalloc_cpumask_var(&vmx_tdx.vmx_enabled, GFP_KERNEL)) {
+		r = -ENOMEM;
+		goto out;
+	}
+
 	/* tdx_enable() in tdx_module_setup() requires cpus lock. */
 	cpus_read_lock();
 	/*
@@ -4592,12 +4610,15 @@ int __init tdx_hardware_setup(struct kvm_x86_ops *x86_ops)
 	 */
 	if (!cpumask_equal(cpu_online_mask, cpu_present_mask))
 		pr_warn("The old TDX module requires all present CPUs to be online to initialize.\n");
-	on_each_cpu(vmx_tdx_on, &err, true);	/* TDX requires vmxon. */
-	r = atomic_read(&err);
+	on_each_cpu(vmx_tdx_on, &vmx_tdx, true);	/* TDX requires vmxon. */
+	r = atomic_read(vmx_tdx.err);
 	if (!r)
 		r = tdx_module_setup();
-	on_each_cpu(vmx_off, NULL, true);
+	else
+		r = -EIO;
+	on_each_cpu(vmx_off, vmx_tdx.vmx_enabled, true);
 	cpus_read_unlock();
+	free_cpumask_var(vmx_tdx.vmx_enabled);
 	if (r)
 		goto out;
 


-- 
Isaku Yamahata <isaku.yamahata@xxxxxxxxx>