Re: [PATCH v8 004/103] KVM: VMX: Move out vmx_x86_ops to 'main.c' to wrap VMX and TDX

[Huang, Kai]

On Sun, 2022-08-07 at 15:00 -0700, isaku.yamahata@xxxxxxxxx wrote:
> From: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> 
> KVM accesses Virtual Machine Control Structure (VMCS) with VMX instructions
> to operate on VM.  TDX defines its data structure and TDX SEAMCALL APIs for

				 its own data structures

> VMM to operate on Trust Domain (TD) instead.

And sorry I don't see how below paragraphs from here ...

> 
> Trust Domain Virtual Processor State (TDVPS) is the root control structure
> of a TD VCPU.  It helps the TDX module control the operation of the VCPU,
> and holds the VCPU state while the VCPU is not running. TDVPS is opaque to
> software and DMA access, accessible only by using the TDX module interface
> functions (such as TDH.VP.RD, TDH.VP.WR ,..).  TDVPS includes TD VMCS, and
> TD VMCS auxiliary structures, such as virtual APIC page, virtualization
> exception information, etc.  TDVPS is composed of Trust Domain Virtual
> Processor Root (TDVPR) which is the root page of TDVPS and Trust Domain
> Virtual Processor eXtension (TDVPX) pages which extend TDVPR to help
> provide enough physical space for the logical TDVPS structure.
> 
> Also, we have a new structure, Trust Domain Control Structure (TDCS) is the
> main control structure of a guest TD, and encrypted (using the guest TD's
> ephemeral private key).  At a high level, TDCS holds information for
> controlling TD operation as a whole, execution, EPTP, MSR bitmaps, etc. KVM
> needs to set it up.  Note that MSR bitmaps are held as part of TDCS (unlike
> VMX) because they are meant to have the same value for all VCPUs of the
> same TD.  TDCS is a multi-page logical structure composed of multiple Trust
> Domain Control Extension (TDCX) physical pages.  Trust Domain Root (TDR) is
> the root control structure of a guest TD and is encrypted using the TDX
> global private key. It holds a minimal set of state variables that enable
> guest TD control even during times when the TD's private key is not known,
> or when the TD's key management state does not permit access to memory
> encrypted using the TD's private key.
> 
> The following shows the relationship between those structures.
> 
>         TDR--> TDCS                     per-TD
>          |       \--> TDCX
>          \
>           \--> TDVPS                    per-TD VCPU
>                  \--> TDVPR and TDVPX
> 

... to here are directly related to this patch.  They belong to the patch(es)
which actually implements them.

For instance, I don't think details like "MSR bitmaps are held as part of TDCS
(unlike VMX) because they are meant to have the same value for all VCPUs of the
same TD" are related to this patch.

What we need to justify the change here is simple: 

TDX doesn't allow VMM to operate VMCS directly.  Instead, TDX has its own data
structures, and uses SEAMCALLs to operate those data structures.  This means we
must have a TDX version of kvm_x86_ops.

I think your first paragraph kinda already does this.

Then you can balabala below ...


> The existing global struct kvm_x86_ops already defines an interface which
> fits with TDX.  But kvm_x86_ops is system-wide, not per-VM structure.  To
> allow VMX to coexist with TDs, the kvm_x86_ops callbacks will have wrappers
> "if (tdx) tdx_op() else vmx_op()" to switch VMX or TDX at run time.
> 
> To split the runtime switch, the VMX implementation, and the TDX
> implementation, add main.c, and move out the vmx_x86_ops hooks in
> preparation for adding TDX, which can coexist with VMX, i.e. KVM can run
> both VMs and TDs.  Use 'vt' for the naming scheme as a nod to VT-x and as a
> concatenation of VmxTdx.
> 
> The current code looks as follows.
> In vmx.c
>   static vmx_op() { ... }
>   static struct kvm_x86_ops vmx_x86_ops = {
>         .op = vmx_op,
>   initialization code
> 
> The eventually converted code will look like
> In vmx.c, keep the VMX operations.
>   vmx_op() { ... }
>   VMX initialization
> In tdx.c, define the TDX operations.
>   tdx_op() { ... }
>   TDX initialization
> In x86_ops.h, declare the VMX and TDX operations.
>   vmx_op();
>   tdx_op();
> In main.c, define common wrappers for VMX and VMX.
>   static vt_ops() { if (tdx) tdx_ops() else vmx_ops() }
>   static struct kvm_x86_ops vt_x86_ops = {
>         .op = vt_op,
>   initialization to call VMX and TDX initialization
> 
> Opportunistically, fix the name inconsistency from vmx_create_vcpu() and
> vmx_free_vcpu() to vmx_vcpu_create() and vxm_vcpu_free().
> 
> Co-developed-by: Xiaoyao Li <xiaoyao.li@xxxxxxxxx>
> Signed-off-by: Xiaoyao Li <xiaoyao.li@xxxxxxxxx>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> Signed-off-by: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> Reviewed-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> ---
>  arch/x86/kvm/Makefile      |   2 +-
>  arch/x86/kvm/vmx/main.c    | 155 ++++++++++++++++
>  arch/x86/kvm/vmx/vmx.c     | 363 +++++++++++--------------------------
>  arch/x86/kvm/vmx/x86_ops.h | 125 +++++++++++++
>  4 files changed, 386 insertions(+), 259 deletions(-)
>  create mode 100644 arch/x86/kvm/vmx/main.c
>  create mode 100644 arch/x86/kvm/vmx/x86_ops.h