Received-SPF: pass (google.com: domain of linux-kernel+bounces-112077-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Fri, 22 Mar 2024 15:45:31 -0700
From: Isaku Yamahata <isaku.yamahata@intel.com>
To: "Huang, Kai" <kai.huang@intel.com>
Cc: "Yamahata, Isaku" <isaku.yamahata@intel.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"isaku.yamahata@gmail.com" <isaku.yamahata@gmail.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	"Aktas, Erdem" <erdemaktas@google.com>,
	Sean Christopherson <seanjc@google.com>,
	Sagi Shahar <sagis@google.com>, "Chen, Bo2" <chen.bo@intel.com>,
	"Yuan, Hang" <hang.yuan@intel.com>,
	"Zhang, Tina" <tina.zhang@intel.com>,
	isaku.yamahata@linux.intel.com
Subject: Re: [PATCH v19 024/130] KVM: TDX: Add placeholders for TDX VM/vcpu
 structure
Message-ID: <20240322224531.GB1994522@ls.amr.corp.intel.com>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <c857863a346e692837b0c35da8a0e03c45311496.1708933498.git.isaku.yamahata@intel.com>
 <dd389847-6f67-4f5d-8358-5d6b6a493797@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <dd389847-6f67-4f5d-8358-5d6b6a493797@intel.com>

On Fri, Mar 22, 2024 at 10:37:20AM +1300,
"Huang, Kai" <kai.huang@intel.com> wrote:

> 
> 
> 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;
> 	...
> };

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 <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.

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.
-- 
Isaku Yamahata <isaku.yamahata@intel.com>