Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 7 Jun 2023 11:06:16 -0700
From:   Isaku Yamahata <isaku.yamahata@gmail.com>
To:     "Huang, Kai" <kai.huang@intel.com>
Cc:     "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "Yamahata, Isaku" <isaku.yamahata@intel.com>,
        "Christopherson,, Sean" <seanjc@google.com>,
        "Chen, Bo2" <chen.bo@intel.com>, "Shahar, Sagi" <sagis@google.com>,
        "isaku.yamahata@gmail.com" <isaku.yamahata@gmail.com>,
        "Aktas, Erdem" <erdemaktas@google.com>,
        "pbonzini@redhat.com" <pbonzini@redhat.com>,
        "dmatlack@google.com" <dmatlack@google.com>,
        "zhi.wang.linux@gmail.com" <zhi.wang.linux@gmail.com>
Subject: Re: [PATCH v14 004/113] KVM: TDX: Initialize the TDX module when
 loading the KVM intel kernel module
Message-ID: <20230607180616.GF2244082@ls.amr.corp.intel.com>
References: <cover.1685333727.git.isaku.yamahata@intel.com>
 <e628e2d235d9b6c00b9bd5d81bb69136b77d13c4.1685333727.git.isaku.yamahata@intel.com>
 <3d8355ae582119486ac7ec129c4ba18d2cdf3ccb.camel@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <3d8355ae582119486ac7ec129c4ba18d2cdf3ccb.camel@intel.com>
Precedence: bulk

On Tue, Jun 06, 2023 at 04:19:33AM +0000,
"Huang, Kai" <kai.huang@intel.com> 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@gmail.com>