Re: [PATCH v17 001/116] KVM: VMX: Move out vmx_x86_ops to 'main.c' to wrap VMX and TDX

[Binbin Wu]

On 11/7/2023 10:55 PM, 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 doesn't allow VMM to operate VMCS directly.
> Instead, TDX has its own data structures, and TDX SEAMCALL APIs for VMM to
> indirectly operate those data structures.  This means we must have a TDX
> version of kvm_x86_ops.
>
> 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 TDX.
>    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().

Nit: 3 indentation alignment below.

Reviewed-by: Binbin Wu <binbin.wu@xxxxxxxxxxxxxxx>

> 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    | 167 +++++++++++++++++
>   arch/x86/kvm/vmx/vmx.c     | 374 ++++++++++---------------------------
>   arch/x86/kvm/vmx/x86_ops.h | 125 +++++++++++++
>   4 files changed, 395 insertions(+), 273 deletions(-)
>   create mode 100644 arch/x86/kvm/vmx/main.c
>   create mode 100644 arch/x86/kvm/vmx/x86_ops.h
>
> diff --git a/arch/x86/kvm/Makefile b/arch/x86/kvm/Makefile
> index 80e3fe184d17..0e894ae23cbc 100644
> --- a/arch/x86/kvm/Makefile
> +++ b/arch/x86/kvm/Makefile
> @@ -23,7 +23,7 @@ kvm-$(CONFIG_KVM_XEN)	+= xen.o
>   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/hyperv.o vmx/nested.o vmx/posted_intr.o vmx/main.o
>   kvm-intel-$(CONFIG_X86_SGX_KVM)	+= vmx/sgx.o
>   
>   kvm-amd-y		+= svm/svm.o svm/vmenter.o svm/pmu.o svm/nested.o svm/avic.o \
> diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
[...]
>   
> -static int vmx_check_emulate_instruction(struct kvm_vcpu *vcpu, int emul_type,
> +int vmx_check_emulate_instruction(struct kvm_vcpu *vcpu, int emul_type,
>   					 void *insn, int insn_len)
Indentaion alignment

>   {
>   	/*
> @@ -1742,7 +1740,7 @@ static int skip_emulated_instruction(struct kvm_vcpu *vcpu)
>    * Recognizes a pending MTF VM-exit and records the nested state for later
>    * delivery.
>    */
> -static void vmx_update_emulated_instruction(struct kvm_vcpu *vcpu)
> +void vmx_update_emulated_instruction(struct kvm_vcpu *vcpu)
>   {
>   	struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
>   	struct vcpu_vmx *vmx = to_vmx(vcpu);
> @@ -1773,7 +1771,7 @@ static void vmx_update_emulated_instruction(struct kvm_vcpu *vcpu)
>   	}
>   }
>   
[...]
> +u8 vmx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio);
> +void vmx_get_exit_info(struct kvm_vcpu *vcpu, u32 *reason,
> +		u64 *info1, u64 *info2, u32 *intr_info, u32 *error_code);
Indentation alignment

> +u64 vmx_get_l2_tsc_offset(struct kvm_vcpu *vcpu);
> +u64 vmx_get_l2_tsc_multiplier(struct kvm_vcpu *vcpu);
> +void vmx_write_tsc_offset(struct kvm_vcpu *vcpu);
> +void vmx_write_tsc_multiplier(struct kvm_vcpu *vcpu);
> +void vmx_request_immediate_exit(struct kvm_vcpu *vcpu);
> +void vmx_sched_in(struct kvm_vcpu *vcpu, int cpu);
> +void vmx_update_cpu_dirty_logging(struct kvm_vcpu *vcpu);
> +#ifdef CONFIG_X86_64
> +int vmx_set_hv_timer(struct kvm_vcpu *vcpu, u64 guest_deadline_tsc,
> +		bool *expired);

Indentation alignment

> +void vmx_cancel_hv_timer(struct kvm_vcpu *vcpu);
> +#endif
> +void vmx_setup_mce(struct kvm_vcpu *vcpu);
> +
> +#endif /* __KVM_X86_VMX_X86_OPS_H */