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

Huang, Kai
2024-03-21T21:37:52Z

On 26/02/2024 9:25 pm, Yamahata, Isaku wrote:
> From: Isaku Yamahata <isaku.yamahata@intel.com>
>
> 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 <isaku.yamahata@intel.com>
>
> ---
> 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 <isaku.yamahata@intel.com>
> ---
> 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;