[v19,024/130] KVM: TDX: Add placeholders for TDX VM/vcpu structure

Commit Message

Isaku Yamahata Feb. 26, 2024, 8:25 a.m. UTC

From: Isaku Yamahata <[e-mail]>

Add placeholders TDX VM/vcpu structure that overlays with VMX VM/vcpu
structures.  Initialize VM structure size and vcpu size/align so that x86
KVM common code knows those size irrespective of VMX or TDX.  Those
structures will be populated as guest creation logic develops.

Add helper functions to check if the VM is guest TD and add conversion
functions between KVM VM/VCPU and TDX VM/VCPU.

Signed-off-by: Isaku Yamahata <[e-mail]>

---
v19:
- correctly update ops.vm_size, vcpu_size and, vcpu_align by Xiaoyao

v14 -> v15:
- use KVM_X86_TDX_VM

Signed-off-by: Isaku Yamahata <[e-mail]>
---
 arch/x86/kvm/vmx/main.c | 14 ++++++++++++
 arch/x86/kvm/vmx/tdx.c  |  1 +
 arch/x86/kvm/vmx/tdx.h  | 50 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 65 insertions(+)
 create mode 100644 arch/x86/kvm/vmx/tdx.h

Comments

Binbin Wu March 14, 2024, 6:21 a.m. UTC | #1

On 2/26/2024 4:25 PM, [e-mail] wrote:
> From: Isaku Yamahata <[e-mail]>
>
> Add placeholders TDX VM/vcpu structure that overlays with VMX VM/vcpu
> structures.  Initialize VM structure size and vcpu size/align so that x86
> KVM common code knows those size irrespective of VMX or TDX.  Those
> structures will be populated as guest creation logic develops.
>
> Add helper functions to check if the VM is guest TD and add conversion
> functions between KVM VM/VCPU and TDX VM/VCPU.
>
> Signed-off-by: Isaku Yamahata <[e-mail]>
>
> ---
> v19:
> - correctly update ops.vm_size, vcpu_size and, vcpu_align by Xiaoyao
>
> v14 -> v15:
> - use KVM_X86_TDX_VM
>
> Signed-off-by: Isaku Yamahata <[e-mail]>
> ---
>   arch/x86/kvm/vmx/main.c | 14 ++++++++++++
>   arch/x86/kvm/vmx/tdx.c  |  1 +
>   arch/x86/kvm/vmx/tdx.h  | 50 +++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 65 insertions(+)
>   create mode 100644 arch/x86/kvm/vmx/tdx.h
>
> diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
> index 18aef6e23aab..e11edbd19e7c 100644
> --- a/arch/x86/kvm/vmx/main.c
> +++ b/arch/x86/kvm/vmx/main.c
> @@ -5,6 +5,7 @@
>   #include "vmx.h"
>   #include "nested.h"
>   #include "pmu.h"
> +#include "tdx.h"
>   
>   static bool enable_tdx __ro_after_init;
>   module_param_named(tdx, enable_tdx, bool, 0444);
> @@ -18,6 +19,9 @@ static __init int vt_hardware_setup(void)
>   		return ret;
>   
>   	enable_tdx = enable_tdx && !tdx_hardware_setup(&vt_x86_ops);
> +	if (enable_tdx)
> +		vt_x86_ops.vm_size = max_t(unsigned int, vt_x86_ops.vm_size,
> +					   sizeof(struct kvm_tdx));
>   
>   	return 0;
>   }
> @@ -215,8 +219,18 @@ static int __init vt_init(void)
>   	 * Common KVM initialization _must_ come last, after this, /dev/kvm is
>   	 * exposed to userspace!
>   	 */
> +	/*
> +	 * kvm_x86_ops is updated with vt_x86_ops.  vt_x86_ops.vm_size must
> +	 * be set before kvm_x86_vendor_init().

The comment is not right?
In this patch, vt_x86_ops.vm_size is set in  vt_hardware_setup(),
which is called in kvm_x86_vendor_init().

Since kvm_x86_ops is updated by kvm_ops_update() with the fields of
vt_x86_ops. I guess you wanted to say vt_x86_ops.vm_size must be set
before kvm_ops_update()?

> +	 */
>   	vcpu_size = sizeof(struct vcpu_vmx);
>   	vcpu_align = __alignof__(struct vcpu_vmx);
> +	if (enable_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));
> +	}
>   	r = kvm_init(vcpu_size, vcpu_align, THIS_MODULE);
>   	if (r)
>   		goto err_kvm_init;
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index 43c504fb4fed..14ef0ccd8f1a 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -6,6 +6,7 @@
>   #include "capabilities.h"
>   #include "x86_ops.h"
>   #include "x86.h"
> +#include "tdx.h"
>   
>   #undef pr_fmt
>   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> diff --git a/arch/x86/kvm/vmx/tdx.h b/arch/x86/kvm/vmx/tdx.h
> new file mode 100644
> index 000000000000..473013265bd8
> --- /dev/null
> +++ b/arch/x86/kvm/vmx/tdx.h
> @@ -0,0 +1,50 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __KVM_X86_TDX_H
> +#define __KVM_X86_TDX_H
> +
> +#ifdef CONFIG_INTEL_TDX_HOST
> +struct kvm_tdx {
> +	struct kvm kvm;
> +	/* TDX specific members follow. */
> +};
> +
> +struct vcpu_tdx {
> +	struct kvm_vcpu	vcpu;
> +	/* TDX specific members follow. */
> +};
> +
> +static inline bool is_td(struct kvm *kvm)
> +{
> +	return kvm->arch.vm_type == KVM_X86_TDX_VM;
> +}
> +
> +static inline bool is_td_vcpu(struct kvm_vcpu *vcpu)
> +{
> +	return is_td(vcpu->kvm);
> +}
> +
> +static inline struct kvm_tdx *to_kvm_tdx(struct kvm *kvm)
> +{
> +	return container_of(kvm, struct kvm_tdx, kvm);
> +}
> +
> +static inline struct vcpu_tdx *to_tdx(struct kvm_vcpu *vcpu)
> +{
> +	return container_of(vcpu, struct vcpu_tdx, vcpu);
> +}
> +#else
> +struct kvm_tdx {
> +	struct kvm kvm;
> +};
> +
> +struct vcpu_tdx {
> +	struct kvm_vcpu	vcpu;
> +};
> +
> +static inline bool is_td(struct kvm *kvm) { return false; }
> +static inline bool is_td_vcpu(struct kvm_vcpu *vcpu) { return false; }
> +static inline struct kvm_tdx *to_kvm_tdx(struct kvm *kvm) { return NULL; }
> +static inline struct vcpu_tdx *to_tdx(struct kvm_vcpu *vcpu) { return NULL; }
> +#endif /* CONFIG_INTEL_TDX_HOST */
> +
> +#endif /* __KVM_X86_TDX_H */

Isaku Yamahata March 14, 2024, 4:37 p.m. UTC | #2

On Thu, Mar 14, 2024 at 02:21:04PM +0800,
Binbin Wu <[e-mail]> wrote:

> > diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
> > index 18aef6e23aab..e11edbd19e7c 100644
> > --- a/arch/x86/kvm/vmx/main.c
> > +++ b/arch/x86/kvm/vmx/main.c
> > @@ -5,6 +5,7 @@
> >   #include "vmx.h"
> >   #include "nested.h"
> >   #include "pmu.h"
> > +#include "tdx.h"
> >   static bool enable_tdx __ro_after_init;
> >   module_param_named(tdx, enable_tdx, bool, 0444);
> > @@ -18,6 +19,9 @@ static __init int vt_hardware_setup(void)
> >   		return ret;
> >   	enable_tdx = enable_tdx && !tdx_hardware_setup(&vt_x86_ops);
> > +	if (enable_tdx)
> > +		vt_x86_ops.vm_size = max_t(unsigned int, vt_x86_ops.vm_size,
> > +					   sizeof(struct kvm_tdx));
> >   	return 0;
> >   }
> > @@ -215,8 +219,18 @@ static int __init vt_init(void)
> >   	 * Common KVM initialization _must_ come last, after this, /dev/kvm is
> >   	 * exposed to userspace!
> >   	 */
> > +	/*
> > +	 * kvm_x86_ops is updated with vt_x86_ops.  vt_x86_ops.vm_size must
> > +	 * be set before kvm_x86_vendor_init().
> 
> The comment is not right?
> In this patch, vt_x86_ops.vm_size is set in  vt_hardware_setup(),
> which is called in kvm_x86_vendor_init().
> 
> Since kvm_x86_ops is updated by kvm_ops_update() with the fields of
> vt_x86_ops. I guess you wanted to say vt_x86_ops.vm_size must be set
> before kvm_ops_update()?

Correct. Here's an updated version.

       /*
        * vt_hardware_setup() updates vt_x86_ops.  Because kvm_ops_update()
        * copies vt_x86_ops to kvm_x86_op, vt_x86_ops must be updated before
        * kvm_ops_update() called by kvm_x86_vendor_init().
        */

Huang, Kai March 21, 2024, 9:37 p.m. UTC | #3

On 26/02/2024 9:25 pm, Yamahata, Isaku wrote:
> From: Isaku Yamahata <[e-mail]>
> 
> Add placeholders TDX VM/vcpu structure that overlays with VMX VM/vcpu
> structures.  Initialize VM structure size and vcpu size/align so that x86
> KVM common code knows those size irrespective of VMX or TDX.  Those
> structures will be populated as guest creation logic develops.
> 
> Add helper functions to check if the VM is guest TD and add conversion
> functions between KVM VM/VCPU and TDX VM/VCPU.

The changelog is essentially only saying "doing what" w/o "why".

Please at least explain why you invented the 'struct kvm_tdx' and 
'struct vcpu_tdx', and why they are invented in this way.

E.g., can we extend 'struct kvm_vmx' for TDX?

struct kvm_tdx {
	struct kvm_vmx vmx;
	...
};

> 
> Signed-off-by: Isaku Yamahata <[e-mail]>
> 
> ---
> v19:
> - correctly update ops.vm_size, vcpu_size and, vcpu_align by Xiaoyao
> 
> v14 -> v15:
> - use KVM_X86_TDX_VM
> 
> Signed-off-by: Isaku Yamahata <[e-mail]>
> ---
>   arch/x86/kvm/vmx/main.c | 14 ++++++++++++
>   arch/x86/kvm/vmx/tdx.c  |  1 +
>   arch/x86/kvm/vmx/tdx.h  | 50 +++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 65 insertions(+)
>   create mode 100644 arch/x86/kvm/vmx/tdx.h
> 
> diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
> index 18aef6e23aab..e11edbd19e7c 100644
> --- a/arch/x86/kvm/vmx/main.c
> +++ b/arch/x86/kvm/vmx/main.c
> @@ -5,6 +5,7 @@
>   #include "vmx.h"
>   #include "nested.h"
>   #include "pmu.h"
> +#include "tdx.h"
>   
>   static bool enable_tdx __ro_after_init;
>   module_param_named(tdx, enable_tdx, bool, 0444);
> @@ -18,6 +19,9 @@ static __init int vt_hardware_setup(void)
>   		return ret;
>   
>   	enable_tdx = enable_tdx && !tdx_hardware_setup(&vt_x86_ops);
> +	if (enable_tdx)
> +		vt_x86_ops.vm_size = max_t(unsigned int, vt_x86_ops.vm_size,
> +					   sizeof(struct kvm_tdx));
>   

Now I see why you included 'struct kvm_x86_ops' as function parameter.

Please move it to this patch.

>   	return 0;
>   }
> @@ -215,8 +219,18 @@ static int __init vt_init(void)
>   	 * Common KVM initialization _must_ come last, after this, /dev/kvm is
>   	 * exposed to userspace!
>   	 */
> +	/*
> +	 * kvm_x86_ops is updated with vt_x86_ops.  vt_x86_ops.vm_size must
> +	 * be set before kvm_x86_vendor_init().
> +	 */
>   	vcpu_size = sizeof(struct vcpu_vmx);
>   	vcpu_align = __alignof__(struct vcpu_vmx);
> +	if (enable_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));
> +	}

Since you are updating vm_size in vt_hardware_setup(), I am wondering 
whether we can do similar thing for vcpu_size and vcpu_align.

That is, we put them both to 'struct kvm_x86_ops', and you update them 
in vt_hardware_setup().

kvm_init() can then just access them directly in this way both 
'vcpu_size' and 'vcpu_align' function parameters can be removed.


>   	r = kvm_init(vcpu_size, vcpu_align, THIS_MODULE);
>   	if (r)
>   		goto err_kvm_init;

Isaku Yamahata March 22, 2024, 10:45 p.m. UTC | #4

On Fri, Mar 22, 2024 at 10:37:20AM +1300,
"Huang, Kai" <[e-mail]> wrote:

> 
> 
> On 26/02/2024 9:25 pm, Yamahata, Isaku wrote:
> > From: Isaku Yamahata <[e-mail]>
> > 
> > Add placeholders TDX VM/vcpu structure that overlays with VMX VM/vcpu
> > structures.  Initialize VM structure size and vcpu size/align so that x86
> > KVM common code knows those size irrespective of VMX or TDX.  Those
> > structures will be populated as guest creation logic develops.
> > 
> > Add helper functions to check if the VM is guest TD and add conversion
> > functions between KVM VM/VCPU and TDX VM/VCPU.
> 
> The changelog is essentially only saying "doing what" w/o "why".
> 
> Please at least explain why you invented the 'struct kvm_tdx' and 'struct
> vcpu_tdx', and why they are invented in this way.
> 
> E.g., can we extend 'struct kvm_vmx' for TDX?
> 
> struct kvm_tdx {
> 	struct kvm_vmx vmx;
> 	...
> };

Here is the updated version.

KVM: TDX: Add placeholders for TDX VM/vcpu structure

Add placeholders TDX VM/vCPU structure, overlaying with the existing
VMX VM/vCPU structures.  Initialize VM structure size and vCPU
size/align so that x86 KVM-common code knows those sizes irrespective
of VMX or TDX.  Those structures will be populated as guest creation
logic develops.

TDX requires its data structure for guest and vcpu.  For VMX, we
already have struct kvm_vmx and struct vcpu_vmx.  Two options to add
TDX-specific members.

1. Append TDX-specific members to kvm_vmx and vcpu_vmx.  Use the same
    struct for both VMX and TDX.
2. Define TDX-specific data struct and overlay.

Choose option two because it has less memory overhead and what member
is needed is clearer

Add helper functions to check if the VM is guest TD and add the conversion
functions between KVM VM/vCPU and TDX VM/vCPU.


> > Signed-off-by: Isaku Yamahata <[e-mail]>
> > 
> > ---
> > v19:
> > - correctly update ops.vm_size, vcpu_size and, vcpu_align by Xiaoyao
> > 
> > v14 -> v15:
> > - use KVM_X86_TDX_VM
> > 
> > Signed-off-by: Isaku Yamahata <[e-mail]>
> > ---
> >   arch/x86/kvm/vmx/main.c | 14 ++++++++++++
> >   arch/x86/kvm/vmx/tdx.c  |  1 +
> >   arch/x86/kvm/vmx/tdx.h  | 50 +++++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 65 insertions(+)
> >   create mode 100644 arch/x86/kvm/vmx/tdx.h
> > 
> > diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
> > index 18aef6e23aab..e11edbd19e7c 100644
> > --- a/arch/x86/kvm/vmx/main.c
> > +++ b/arch/x86/kvm/vmx/main.c
> > @@ -5,6 +5,7 @@
> >   #include "vmx.h"
> >   #include "nested.h"
> >   #include "pmu.h"
> > +#include "tdx.h"
> >   static bool enable_tdx __ro_after_init;
> >   module_param_named(tdx, enable_tdx, bool, 0444);
> > @@ -18,6 +19,9 @@ static __init int vt_hardware_setup(void)
> >   		return ret;
> >   	enable_tdx = enable_tdx && !tdx_hardware_setup(&vt_x86_ops);
> > +	if (enable_tdx)
> > +		vt_x86_ops.vm_size = max_t(unsigned int, vt_x86_ops.vm_size,
> > +					   sizeof(struct kvm_tdx));
> 
> Now I see why you included 'struct kvm_x86_ops' as function parameter.
> 
> Please move it to this patch.

Sure.

> >   	return 0;
> >   }
> > @@ -215,8 +219,18 @@ static int __init vt_init(void)
> >   	 * Common KVM initialization _must_ come last, after this, /dev/kvm is
> >   	 * exposed to userspace!
> >   	 */
> > +	/*
> > +	 * kvm_x86_ops is updated with vt_x86_ops.  vt_x86_ops.vm_size must
> > +	 * be set before kvm_x86_vendor_init().
> > +	 */
> >   	vcpu_size = sizeof(struct vcpu_vmx);
> >   	vcpu_align = __alignof__(struct vcpu_vmx);
> > +	if (enable_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));
> > +	}
> 
> Since you are updating vm_size in vt_hardware_setup(), I am wondering
> whether we can do similar thing for vcpu_size and vcpu_align.
> 
> That is, we put them both to 'struct kvm_x86_ops', and you update them in
> vt_hardware_setup().
> 
> kvm_init() can then just access them directly in this way both 'vcpu_size'
> and 'vcpu_align' function parameters can be removed.

Hmm, now I noticed the vm_size can be moved here.  We have

 	vcpu_size = sizeof(struct vcpu_vmx);
 	vcpu_align = __alignof__(struct vcpu_vmx);
	if (enable_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));
                vt_x86_ops.vm_size = max_t(unsigned int, vt_x86_ops.vm_size,
                                          sizeof(struct kvm_tdx));
	}


We can add vcpu_size, vcpu_align to struct kvm_x86_ops. If we do so, we have
to touch svm code unnecessarily.

Huang, Kai March 25, 2024, 10:22 a.m. UTC | #5

> 
> Here is the updated version.
> 
> KVM: TDX: Add placeholders for TDX VM/vcpu structure
> 
> Add placeholders TDX VM/vCPU structure, overlaying with the existing

			       ^ structures

"TDX VM/vCPU structure" -> "TDX VM/vCPU structures".

And I don't quite understand what does "overlaying" mean here.

> VMX VM/vCPU structures.  Initialize VM structure size and vCPU
> size/align so that x86 KVM-common code knows those sizes irrespective
> of VMX or TDX.  Those structures will be populated as guest creation
> logic develops.
> 
> TDX requires its data structure for guest and vcpu.  For VMX, we

I don't think TDX "requires" anything here.  Introducing separate structures are
software implementation, but not requirement by TDX.

> already have struct kvm_vmx and struct vcpu_vmx.  Two options to add
> TDX-specific members.
> 
> 1. Append TDX-specific members to kvm_vmx and vcpu_vmx.  Use the same
>     struct for both VMX and TDX.
> 2. Define TDX-specific data struct and overlay.
> 
> Choose option two because it has less memory overhead and what member
> is needed is clearer
> 
> Add helper functions to check if the VM is guest TD and add the conversion
> functions between KVM VM/vCPU and TDX VM/vCPU.

FYI:

Add TDX's own VM and vCPU structures as placeholder to manage and run TDX
guests.

TDX protects guest VMs from malicious host.  Unlike VMX guests, TDX guests are
crypto-protected.  KVM cannot access TDX guests' memory and vCPU states
directly.  Instead, TDX requires KVM to use a set of architecture-defined
firmware APIs (a.k.a TDX module SEAMCALLs) to manage and run TDX guests.

In fact, the way to manage and run TDX guests and normal VMX guests are quite
different.  Because of that, the current structures ('struct kvm_vmx' and
'struct vcpu_vmx') to manage VMX guests are not quite suitable for TDX guests. 
E.g., the majority of the members of 'struct vcpu_vmx' don't apply to TDX
guests.

Introduce TDX's own VM and vCPU structures ('struct kvm_tdx' and 'struct
vcpu_tdx' respectively) for KVM to manage and run TDX guests.  And instead of
building TDX's VM and vCPU structures based on VMX's, build them directly based
on 'struct kvm'.

As a result, TDX and VMX will have different VM size and vCPU size/alignment. 
Adjust the 'vt_x86_ops.vm_size' and the 'vcpu_size' and 'vcpu_align' to the
maximum value of TDX guest and VMX guest during module initialization time so
that KVM can always allocate enough memory for both TDX guests and VMX guests.

[...]

> > 
> > > @@ -215,8 +219,18 @@ static int __init vt_init(void)
> > >   	 * Common KVM initialization _must_ come last, after this, /dev/kvm is
> > >   	 * exposed to userspace!
> > >   	 */
> > > +	/*
> > > +	 * kvm_x86_ops is updated with vt_x86_ops.  vt_x86_ops.vm_size must
> > > +	 * be set before kvm_x86_vendor_init().
> > > +	 */
> > >   	vcpu_size = sizeof(struct vcpu_vmx);
> > >   	vcpu_align = __alignof__(struct vcpu_vmx);
> > > +	if (enable_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));
> > > +	}
> > 
> > Since you are updating vm_size in vt_hardware_setup(), I am wondering
> > whether we can do similar thing for vcpu_size and vcpu_align.
> > 
> > That is, we put them both to 'struct kvm_x86_ops', and you update them in
> > vt_hardware_setup().
> > 
> > kvm_init() can then just access them directly in this way both 'vcpu_size'
> > and 'vcpu_align' function parameters can be removed.
> 
> Hmm, now I noticed the vm_size can be moved here.  We have
> 
>  	vcpu_size = sizeof(struct vcpu_vmx);
>  	vcpu_align = __alignof__(struct vcpu_vmx);
> 	if (enable_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));
>                 vt_x86_ops.vm_size = max_t(unsigned int, vt_x86_ops.vm_size,
>                                           sizeof(struct kvm_tdx));
> 	}
> 
> 
> We can add vcpu_size, vcpu_align to struct kvm_x86_ops. If we do so, we have
> to touch svm code unnecessarily.

Not only SVM, but also other architectures, because you are going to remove two
function parameters from kvm_init().

That reminds me that other ARCHs may not use 'kvm_x86_ops'-similar thing, so to
make thing simple I am fine with your above approach.

Huang, Kai April 23, 2024, 1:59 p.m. UTC | #6

On Fri, 2024-03-22 at 15:45 -0700, Isaku Yamahata wrote:
> Hmm, now I noticed the vm_size can be moved here.  We have
> 
>  	vcpu_size = sizeof(struct vcpu_vmx);
>  	vcpu_align = __alignof__(struct vcpu_vmx);
> 	if (enable_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));
>                 vt_x86_ops.vm_size = max_t(unsigned int, vt_x86_ops.vm_size,
>                                           sizeof(struct kvm_tdx));
> 	}

Hmm.. After reading again, I don't think we can?

In your comments that was replied to Binbin:

/*
 * vt_hardware_setup() updates vt_x86_ops.  Because kvm_ops_update()
 * copies vt_x86_ops to kvm_x86_op, vt_x86_ops must be updated before
 * kvm_ops_update() called by kvm_x86_vendor_init().
 */

You said update to vt_x86_ops.vm_size must be done in vt_hardware_setup().

But I think we should move the above comment to the vt_hardware_setup()
where vm_size is truly updated.

	/*
	 * TDX and VMX have different VM structure.  If TDX is enabled,
	 * update vt_x86_ops.vm_size to the maximum value of the two
	 * before it is copied to kvm_x86_ops in kvm_update_ops() to make
	 * sure KVM always allocates enough memory for the VM structure.
	 */

Here the purpose to calculate vcpu_size/vcpu_align is just to pass them to
kvm_init().  If needed, we can add a comment to describe "what this code
does":

	/*
	 * TDX and VMX have different vCPU structure.  Calculate the 
	 * maximum size/align so that kvm_init() can use the larger 
	 * values to create the vCPU kmem_cache.
	 */

Patch

diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
index 18aef6e23aab..e11edbd19e7c 100644
--- a/arch/x86/kvm/vmx/main.c
+++ b/arch/x86/kvm/vmx/main.c
@@ -5,6 +5,7 @@ 
 #include "vmx.h"
 #include "nested.h"
 #include "pmu.h"
+#include "tdx.h"
 
 static bool enable_tdx __ro_after_init;
 module_param_named(tdx, enable_tdx, bool, 0444);
@@ -18,6 +19,9 @@  static __init int vt_hardware_setup(void)
 		return ret;
 
 	enable_tdx = enable_tdx && !tdx_hardware_setup(&vt_x86_ops);
+	if (enable_tdx)
+		vt_x86_ops.vm_size = max_t(unsigned int, vt_x86_ops.vm_size,
+					   sizeof(struct kvm_tdx));
 
 	return 0;
 }
@@ -215,8 +219,18 @@  static int __init vt_init(void)
 	 * Common KVM initialization _must_ come last, after this, /dev/kvm is
 	 * exposed to userspace!
 	 */
+	/*
+	 * kvm_x86_ops is updated with vt_x86_ops.  vt_x86_ops.vm_size must
+	 * be set before kvm_x86_vendor_init().
+	 */
 	vcpu_size = sizeof(struct vcpu_vmx);
 	vcpu_align = __alignof__(struct vcpu_vmx);
+	if (enable_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));
+	}
 	r = kvm_init(vcpu_size, vcpu_align, THIS_MODULE);
 	if (r)
 		goto err_kvm_init;
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 43c504fb4fed..14ef0ccd8f1a 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -6,6 +6,7 @@ 
 #include "capabilities.h"
 #include "x86_ops.h"
 #include "x86.h"
+#include "tdx.h"
 
 #undef pr_fmt
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
diff --git a/arch/x86/kvm/vmx/tdx.h b/arch/x86/kvm/vmx/tdx.h
new file mode 100644
index 000000000000..473013265bd8
--- /dev/null
+++ b/arch/x86/kvm/vmx/tdx.h
@@ -0,0 +1,50 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __KVM_X86_TDX_H
+#define __KVM_X86_TDX_H
+
+#ifdef CONFIG_INTEL_TDX_HOST
+struct kvm_tdx {
+	struct kvm kvm;
+	/* TDX specific members follow. */
+};
+
+struct vcpu_tdx {
+	struct kvm_vcpu	vcpu;
+	/* TDX specific members follow. */
+};
+
+static inline bool is_td(struct kvm *kvm)
+{
+	return kvm->arch.vm_type == KVM_X86_TDX_VM;
+}
+
+static inline bool is_td_vcpu(struct kvm_vcpu *vcpu)
+{
+	return is_td(vcpu->kvm);
+}
+
+static inline struct kvm_tdx *to_kvm_tdx(struct kvm *kvm)
+{
+	return container_of(kvm, struct kvm_tdx, kvm);
+}
+
+static inline struct vcpu_tdx *to_tdx(struct kvm_vcpu *vcpu)
+{
+	return container_of(vcpu, struct vcpu_tdx, vcpu);
+}
+#else
+struct kvm_tdx {
+	struct kvm kvm;
+};
+
+struct vcpu_tdx {
+	struct kvm_vcpu	vcpu;
+};
+
+static inline bool is_td(struct kvm *kvm) { return false; }
+static inline bool is_td_vcpu(struct kvm_vcpu *vcpu) { return false; }
+static inline struct kvm_tdx *to_kvm_tdx(struct kvm *kvm) { return NULL; }
+static inline struct vcpu_tdx *to_tdx(struct kvm_vcpu *vcpu) { return NULL; }
+#endif /* CONFIG_INTEL_TDX_HOST */
+
+#endif /* __KVM_X86_TDX_H */