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

[Huang, Kai]

> > 
> > I think you should use hardware_enable_all(), and just do something similar to
> > below in vmx_hardware_enable():
> 
> The use of hardware_enable_all() make us circle back to refactoring KVM
> hardware initialization topic.  I'd like to stay away from it for now for TDX.

Sean's series to improve hardware enable has been merged to upstream already.  

Can you elaborate what's the issue here?

[...]

> +static bool enable_tdx __ro_after_init;
> +module_param_named(tdx, enable_tdx, bool, 0444);
> +
> +static __init int vt_hardware_setup(void)
> +{
> +	int ret;
> +
> +	ret = vmx_hardware_setup();
> +	if (ret)
> +		return ret;
> +
> +	enable_tdx = enable_tdx && !tdx_hardware_setup(&vt_x86_ops);

Unfortunately, the enable_tdx should also be protected by the cpus_read_lock(),
because CPU hotplug code path checks it too (as seen in your next patch).

> +
> +	return 0;
> +}
> +
>  #define VMX_REQUIRED_APICV_INHIBITS		       \
>  (						       \
>         BIT(APICV_INHIBIT_REASON_DISABLE)|	       \
> @@ -159,7 +175,7 @@ struct kvm_x86_ops vt_x86_ops __initdata = {
>  };
>  
>  struct kvm_x86_init_ops vt_init_ops __initdata = {
> -	.hardware_setup = vmx_hardware_setup,
> +	.hardware_setup = vt_hardware_setup,
>  	.handle_intel_pt_intr = NULL,
>  
>  	.runtime_ops = &vt_x86_ops,
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> new file mode 100644
> index 000000000000..8d265d7ae6fb
> --- /dev/null
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -0,0 +1,74 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/cpu.h>
> +
> +#include <asm/tdx.h>
> +
> +#include "capabilities.h"
> +#include "x86_ops.h"
> +#include "x86.h"
> +
> +#undef pr_fmt
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +static int __init tdx_module_setup(void)
> +{
> +	int ret;
> +
> +	ret = tdx_enable();
> +	if (ret) {
> +		pr_info("Failed to initialize TDX module.\n");
> +		return ret;
> +	}
> +
> +	pr_info("TDX is supported.\n");
> +	return 0;
> +}
> +
> +static int __init tdx_cpu_enable_cpu(void *unused)
> +{
> +	int r;
> +
> +	/*
> +	 * TDX requires VMX. Because thread can be migrated, keep VMXON on
> +	 * all online cpus until all TDX module initialization is done.
> +	 */

The second sentence in this comment should be somewhere else, but not here.

> +	r = vmxon();
> +	if (r)
> +		return r;
> +	return tdx_cpu_enable();
> +}
> +
> +static void __init tdx_vmxoff_cpu(void *unused)
> +{
> +	WARN_ON_ONCE(cpu_vmxoff());
> +}
> +
> +int __init tdx_hardware_setup(struct kvm_x86_ops *x86_ops)
> +{
> +	int cpu;
> +	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();
> +	/*
> +	 * Because tdx_cpu_enable() acquires spin locks, on_each_cpu()
> +	 * can't be used.
> +	 */

Here you have cpus_read_lock() protection, so tdx_cpu_enable() cannot be called
from CPU hotplug code path when you are doing here.

So, using on_each_cpu() to do tdx_cpu_enable() is OK here, because on one
particular cpu, when it already has taken the spinlock, it cannot receive IPI
anymore which can try to take the spinlock again.

> +	for_each_online_cpu(cpu) {
> +		if (smp_call_on_cpu(cpu, tdx_cpu_enable_cpu, NULL, false)) {
> +			r = -EIO;
> +			break;
> +		}
> +	}
> +	if (!r)
> +		r = tdx_module_setup();
> +	on_each_cpu(tdx_vmxoff_cpu, NULL, 1);
> +	cpus_read_unlock();
> +
> +	return r;
> +}

I think you can merge next patch with this one because they are kinda related.  

Putting them together allows people to review more easily.