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

[Huang, Kai]

On Thu, 2023-01-12 at 08:31 -0800, isaku.yamahata@xxxxxxxxx wrote:
> From: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> 
> TDX requires several initialization steps for KVM to create guest TDs.
> Detect CPU feature, enable VMX (TDX is based on VMX), detect the TDX module
> availability, and initialize it.  This patch implements those steps.

"detect the TDX module" is not needed anymore.

Btw, I guess you should get rid of "This patch ...".  Please see below quoted
txt from https://docs.kernel.org/process/submitting-patches.html :

"
Describe your changes in imperative mood, e.g. “make xyzzy do frotz” instead of
“[This patch] makes xyzzy do frotz” or “[I] changed xyzzy to do frotz”, as if
you are giving orders to the codebase to change its behaviour.
"

> 
> There are several options on when to initialize the TDX module.  A.) kernel
> module loading time, B.) the first guest TD creation time.  A.) was chosen.
> With B.), a user may hit an error of the TDX initialization when trying to
> create the first guest TD.  The machine that fails to initialize the TDX
> module can't boot any guest TD further.  Such failure is undesirable and a
> surprise because the user expects that the machine can accommodate guest
> TD, but actually not.  So A.) is better than B.).
> 
> Introduce a module parameter, enable_tdx, to explicitly enable TDX KVM
> support.  It's off by default to keep same behavior for those who don't use
> TDX.  Implement hardware_setup method to detect TDX feature of CPU.
> Because TDX requires all present CPUs to enable VMX (VMXON).  The x86

"Because TDX ... , the x86 specific ...".

> specific kvm_arch_post_hardware_enable_setup overrides the existing weak
> symbol of kvm_arch_post_hardware_enable_setup which is called at the KVM
> module initialization.
> 
> Suggested-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> Signed-off-by: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> ---
>  arch/x86/kvm/Makefile      |  1 +
>  arch/x86/kvm/vmx/main.c    | 33 +++++++++++++++++++++++-----
>  arch/x86/kvm/vmx/tdx.c     | 44 ++++++++++++++++++++++++++++++++++++++
>  arch/x86/kvm/vmx/vmx.c     | 39 +++++++++++++++++++++++++++++++++
>  arch/x86/kvm/vmx/x86_ops.h | 10 +++++++++
>  5 files changed, 122 insertions(+), 5 deletions(-)
>  create mode 100644 arch/x86/kvm/vmx/tdx.c
> 
> diff --git a/arch/x86/kvm/Makefile b/arch/x86/kvm/Makefile
> index 0e894ae23cbc..4b01ab842ab7 100644
> --- a/arch/x86/kvm/Makefile
> +++ b/arch/x86/kvm/Makefile
> @@ -25,6 +25,7 @@ kvm-$(CONFIG_KVM_SMM)	+= smm.o
>  kvm-intel-y		+= vmx/vmx.o vmx/vmenter.o vmx/pmu_intel.o vmx/vmcs12.o \
>  			   vmx/hyperv.o vmx/nested.o vmx/posted_intr.o vmx/main.o
>  kvm-intel-$(CONFIG_X86_SGX_KVM)	+= vmx/sgx.o
> +kvm-intel-$(CONFIG_INTEL_TDX_HOST)	+= vmx/tdx.o
>  
>  kvm-amd-y		+= svm/svm.o svm/vmenter.o svm/pmu.o svm/nested.o svm/avic.o \
>  			   svm/sev.o svm/hyperv.o
> diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
> index 18f659d1d456..f5d1166d2718 100644
> --- a/arch/x86/kvm/vmx/main.c
> +++ b/arch/x86/kvm/vmx/main.c
> @@ -7,6 +7,22 @@
>  #include "pmu.h"
>  #include "tdx.h"
>  
> +static bool enable_tdx __ro_after_init = IS_ENABLED(CONFIG_INTEL_TDX_HOST);

The changelog said it's off by default, so not consistent here.

> +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);
> +
> +	return 0;
> +}
> +
>  struct kvm_x86_ops vt_x86_ops __initdata = {
>  	.name = KBUILD_MODNAME,
>  
> @@ -149,7 +165,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,
> @@ -182,10 +198,17 @@ static int __init vt_init(void)
>  	 * Common KVM initialization _must_ come last, after this, /dev/kvm is
>  	 * exposed to userspace!
>  	 */
> -	vt_x86_ops.vm_size = max(sizeof(struct kvm_vmx), sizeof(struct kvm_tdx));
> -	vcpu_size = max(sizeof(struct vcpu_vmx), sizeof(struct vcpu_tdx));
> -	vcpu_align = max(__alignof__(struct vcpu_vmx),
> -			 __alignof__(struct vcpu_tdx));
> +	vt_x86_ops.vm_size = sizeof(struct kvm_vmx);
> +	vcpu_size = sizeof(struct vcpu_vmx);
> +	vcpu_align = __alignof__(struct vcpu_vmx);
> +	if (enable_tdx) {
> +		vt_x86_ops.vm_size = max_t(unsigned int, vt_x86_ops.vm_size,
> +					   sizeof(struct kvm_tdx));
> +		vcpu_size = max_t(unsigned int, vcpu_size,
> +				  sizeof(struct vcpu_tdx));
> +		vcpu_align = max_t(unsigned int, vcpu_align,
> +				   __alignof__(struct vcpu_tdx));
> +	}

You have below code in the previous patch:

@@ -181,9 +182,10 @@ static int __init vt_init(void)
 	 * Common KVM initialization _must_ come last, after this, /dev/kvm is
 	 * exposed to userspace!
 	 */
-	vt_x86_ops.vm_size = sizeof(struct kvm_vmx);
-	vcpu_size = sizeof(struct vcpu_vmx);
-	vcpu_align = __alignof__(struct vcpu_vmx);
+	vt_x86_ops.vm_size = max(sizeof(struct kvm_vmx), sizeof(struct
kvm_tdx));
+	vcpu_size = max(sizeof(struct vcpu_vmx), sizeof(struct vcpu_tdx));
+	vcpu_align = max(__alignof__(struct vcpu_vmx),
+			 __alignof__(struct vcpu_tdx));

The chunk here in this patch is not related how to initialize the TDX module,
but belong to "how KVM handles TDX at VM level and vcpu level", which is
introduced in the previous patch, which isn't mandatory to be introduced before
this patch.

IMHO a better way is to move the previous patch after this one, and just put 
this chunk to that patch.  And if you do so, this chunk of change can just
appear once but not twice in two patches.
 

[snip]

>  
> +#ifdef CONFIG_INTEL_TDX_HOST
> +int __init tdx_hardware_setup(struct kvm_x86_ops *x86_ops);
> +#else
> +static inline int tdx_hardware_setup(struct kvm_x86_ops *x86_ops) { return 0; }

Why do you return 0, which is a success IIUC?